Labelbox / labelbox-python

The data factory for next gen AI
https://labelbox.com
Apache License 2.0
118 stars 58 forks source link

User Group Management doesn't support `experimental_enabled` on Client #1808

Closed tommypkeane-gehc closed 1 month ago

tommypkeane-gehc commented 1 month ago

Describe the bug

The Client class has the enable_experimental argument, but the execute() method does not check the self.enable_experimental value.

This means that labelbox.schema.user_group.UserGroup class's methods ignore the enable_experimental even though it's defined as required by that class.

To reproduce

  1. Use method labelbox.schema.user_group.UserGroup()
  2. Pass parameters client=labelbox.Client(api_key="...", enable_experimental=True)
  3. See error: labelbox.exceptions.NetworkError: HTTPSConnectionPool(host='api.labelbox.com', port=443): Max retries exceeded with url: /graphql

Expected behavior

The error should be indicating the experimental URL: /_gql

Screenshots / Videos

N/A

Environment (please complete the following information

Additional context

N/A

Proposed Fix

Every call to self.client.execute(...) like here:

https://github.com/Labelbox/labelbox-python/blob/45109d855c32796a309eae23a9dc53750c3ae9a5/libs/labelbox/src/labelbox/schema/user_group.py#L141

could be updated with self.client.execute(..., experimental=self.client.enable_experimental)

Gabefire commented 1 month ago

Hi, Thank you for reach out! When I run this sample code:

client = lb.Client(api_key=API_KEY, enable_experimental=True)

user = client.get_users()
test = UserGroup(
    client=client,
    name="new group test",
    color=UserGroupColor.BLUE,
    users=set([next(user)]),
).create()

The user group is both initialized and created without error.

The exception you are getting seems like a connection error. If enable_experimental was not enabled it would give this error: https://github.com/Labelbox/labelbox-python/blob/0e9f43f7f948e7702f01cf3cb6c319288ec9655f/libs/labelbox/src/labelbox/schema/user_group.py#L95-L96

Thanks, Gabe

Gabefire commented 1 month ago

Could you run pip freeze and provide the output?

Gabefire commented 1 month ago

Hey @tommypkeane-gehc I reread your comment and going to try and update the URL used by user groups to experimental one.

tommypkeane-gehc commented 1 month ago

Hey @tommypkeane-gehc I reread your comment and going to try and update the URL used by user groups to experimental one.

Thanks, @Gabefire ! Yeah, if these URL differences for the GraphQL requests don't matter, I don't have any particular insight on that. It's just that when using the UserGroup class methods, it would complain that it needed the experimental_enabled like you referenced.

I was experiencing both issues (the other being #1809 ) at the same time, so if everything runs as long as the Client instance has experimental_enabled without needing to change the UserGroup code, then I don't have any particular insight beyond what I shared, so I'd defer to you all about the preferred implementation.

I just noticed that that seemed to be the point of the experimental_enabled and it wasn't actually using that different URL 🤷‍♀️

So if there's a preference not to make the updates here, I don't have any problem with that, as long as the User and UserGroup requests will still work with an experimental_enabled instance of the Client class. And then presumably if there's no difference, like you mention in #1810 then basically that RuntimeError() would eventually go away in a future update, and so the experimental=True for the Client wouldn't be needed in these cases -- am I understanding that correctly?

(And thanks so much for such a quick response!)

Gabefire commented 1 month ago

Hi, We solved this one by removing the experiment tag on user groups. We want to keep that parameter for certain API endpoints but certain SDK methods can be marked experimental while the API endpoint is not.