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

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

Closed
tommypkeane-gehc opened this issue Sep 11, 2024 · 5 comments · Fixed by #1840
Closed

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

tommypkeane-gehc opened this issue Sep 11, 2024 · 5 comments · Fixed by #1840
Assignees

Comments

@tommypkeane-gehc
Copy link

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

  • OS: macOS
  • Python Version: 3.11.x
  • SDK + SDK Version: [email protected]
  • Python Environment: poetry.lock

Additional context

N/A

Proposed Fix

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

result = self.client.execute(query, params)

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

@Gabefire Gabefire self-assigned this Sep 12, 2024
@Gabefire
Copy link
Collaborator

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:

if not self.client.enable_experimental:
raise RuntimeError("Please enable experimental in client to use UserGroups")

Thanks,
Gabe

@Gabefire
Copy link
Collaborator

Could you run pip freeze and provide the output?

@Gabefire
Copy link
Collaborator

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

@tommypkeane-gehc
Copy link
Author

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 Gabefire linked a pull request Sep 24, 2024 that will close this issue
@Gabefire
Copy link
Collaborator

Gabefire commented Oct 2, 2024

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.

@Gabefire Gabefire closed this as completed Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants