Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PT-5198] Workspace singleton #595

Merged
merged 37 commits into from
May 24, 2024
Merged

[PT-5198] Workspace singleton #595

merged 37 commits into from
May 24, 2024

Conversation

andher1802
Copy link
Contributor

@andher1802 andher1802 commented Apr 24, 2024

Described Here

Adding workspace mixing to implement active record pattern

Items:

For release:

  • Bumped version
  • Added changelog

up42/__init__.py Outdated
get_webhooks,
create_webhook,
get_webhook_events,
get_credits_balance,
]
if hasattr(obj, "__name__")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pylint is triggering an error if we do not add this. Alternative option is to set an ignore. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means something is off, otherwise we would get the warning before as well

Copy link
Contributor Author

@andher1802 andher1802 May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some research, I finally found what is off here.

We have changed the authenticate variable to store the method of a Class. It seems that before mypy was able to infer that there were Classes, and functions in the comprenhensive list and all of them have the __name__ property. Now, for some reason (perhaps the new typing, or some update of mypy that has changed since the latest commit that touch the init.py file) mypy is not able to do the same type infer than before.

Since the list contains a mix of classes, functions, and now the new authentication variable. mypy infers the most general type that can accommodate all elements, which is likely object. The object type in Python doesn't have a name attribute, leading to the error.

I figure out that splitting the assingment for Classes, and functions makes mypy happy and infers correctly the type.

I also tested this and it works as well:

__all__ = [
    Asset.__name__,
    Auth.__name__,
    Catalog.__name__,
    Order.__name__,
    Storage.__name__,
    Tasking.__name__,
    Webhook.__name__,
    get_example_aoi.__name__,
    read_vector_file.__name__,
    initialize_catalog.__name__,
    initialize_tasking.__name__,
    initialize_storage.__name__,
    initialize_order.__name__,
    initialize_asset.__name__,
    initialize_webhook.__name__,
    authenticate.__name__,
    get_webhooks.__name__,
    create_webhook.__name__,
    get_webhook_events.__name__,
    get_credits_balance.__name__,
] 

We can think of a cleaner solution, perharps assigning authenticate to a better type. I did some tests but I ended up splitting the assignment of the all property into Classes and functions.

@andher1802 andher1802 marked this pull request as ready for review May 2, 2024 11:39
@andher1802 andher1802 requested a review from javidq May 2, 2024 11:40
@andher1802 andher1802 self-assigned this May 2, 2024
@andher1802 andher1802 requested a review from javidq May 23, 2024 18:18
@javidq javidq changed the title AT5198 Workspace mixin AT5198 Workspace singleton May 24, 2024
@@ -39,9 +39,9 @@ class Storage:
```
"""

def __init__(self, auth: up42_auth.Auth):
def __init__(self, auth: up42_auth.Auth, workspace_id: str):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a breaking change?
I ask because I see the version bumped was set to patch version instead of minor.

@javidq javidq changed the title AT5198 Workspace singleton [PT-5198] Workspace singleton May 24, 2024
Copy link

@andher1802 andher1802 merged commit c3cdf51 into master May 24, 2024
6 checks passed
@andher1802 andher1802 deleted the PT5054_WorkspaceMixin branch May 24, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants