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

Feature/#173 client name #190

Merged
merged 10 commits into from
Feb 3, 2024
Merged

Feature/#173 client name #190

merged 10 commits into from
Feb 3, 2024

Conversation

hemidactylus
Copy link
Collaborator

Closes #173

This proposes simplification and the infrastructure, at the "request functions layer", for optional management of client name and client version by the astrapy classes.

By client_name and client_version it is meant something such as "langchain" and "0.2.0", optionally set by the code that uses astrapy (in a way TBD). This would make for better usage statistics.

  • The APIRequestHandler classes have gone, replaced by functions
  • principle: removal of all default values for params at the utils.py and api.py layers, to let the linters catch forgotten call paths. Default values used in ops.py and db.py, not below.
  • a function(client_name, client_version) to generate the final user-agent string, where both args can be null
  • right now, the db.py and ops.py calls always fill in with None but the infra is in place

As per API specs, it is desired that this client_name is first in a slash-separated string. Currently I went for a format, if everything provided, like "langchain/astrapy/0.7.3/0.2.0".

Open questions

How exactly should code that uses astrapy "set" or communicate this info, if it desires to? (setter methods or additional params to a lot of class methods? Both?). This is to be defined, hence this is a draft.

Please someone come up with a better naming scheme other than client_name/version. There are way too many "clients" around already. On the other hand, "user" seems not right. Caller, perhaps?

@hemidactylus hemidactylus marked this pull request as draft February 2, 2024 00:25
astrapy/utils.py Outdated
def compose_user_agent(
client_name: Optional[str], client_version: Optional[str]
) -> str:
if client_name:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing not ?

Suggested change
if client_name:
if not client_name:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!

@hemidactylus hemidactylus marked this pull request as ready for review February 3, 2024 00:53
@hemidactylus
Copy link
Collaborator Author

I suggest this PR stops here (pure internal refactoring) and another one does the actual implementation of new API.

Besides the internal refactoring (and the final shape of the compose_user_agent function), this PR introduces a new test dependency and a specific test (with a simple example) to intercept requests and check for the user-agent - ready to be extended.

Note: the reason I went for pytest_httpserver and not pytest-httpx is that the latter requires Python 3.9+. We want to keep compatibility with 3.8.

@hemidactylus hemidactylus merged commit e185a05 into master Feb 3, 2024
2 checks passed
@hemidactylus hemidactylus deleted the feature/#173-client-name branch February 3, 2024 07:54
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.

optional "client_name" additional property in clients to specialize user-agent?
3 participants