👨💻 Configure HF Hub URL with environment variable #815
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
Only for testing purposes
There was a problem hiding this comment.
This is certainly an interesting solution to the issue.
I think it's an improvement to what we have, so I'd be happy to merge, but I was thinking of a solution where HfApi accepts a hub url, and that one could pass a HfApi instance to Repository. The latter solution would allow for further customization of HfApi used in Repository.
But I'm happy with this solution anyway.
|
Is d2dba15 more aligned with what you had in mind ? |
SBrandeis
changed the title
SBrandeis
changed the title
| @@ -13,7 +13,7 @@ def get_version() -> str: | |||
|
|
|||
| install_requires = [ | |||
| "filelock", | |||
| "requests", | |||
| "requests>=2.27", | |||
There was a problem hiding this comment.
What makes us require a very recent version of requests? This potentially creates dependency conflicts for users trying to install our library alongside other packages which potentially don't support latest requests versions. What's the oldest version we can work with here?
There was a problem hiding this comment.
Minimal required version of requests is 2.27.0 because we use JSONDecodeError, and it was introduced only in 2.27.0: https://docs.python-requests.org/en/latest/community/updates/#id2
There was a problem hiding this comment.
When was 2.27.0 released? and do we know of libraries that pin an upper bound on the version of requests they use?
There was a problem hiding this comment.
2.27.0 was released on 2022-03-01 2022-01-03
There was a problem hiding this comment.
requests.JSONDecodeError was introduced by psf/requests#5856
There was a problem hiding this comment.
Ok, in my opinion this is ok to ship this version pin, given that most libraries would probably not pin an upper bound on the version of requests they use. And we can always relax this constraint if we realize it's affecting users?
There was a problem hiding this comment.
Let's merge this with this pinned version, I'll open a PR today to fix it and remove the constraint before we release a new version. The pinned version is way too recent. cc @LysandreJik
There was a problem hiding this comment.
Thanks for working on it, @SBrandeis!
[Out of scope for this PR, but some thoughts about passing a HfApi argument to other classes]
I didn't expect HfApi to be an argument to Repository. I think it is an elegant solution to the problem, but I wonder if the name that is given to its instance, hf_api, is the best to describe what it actually does.
To me hf_api describes an API that has methods you can call. But here, we're using it as a stateful client instance to the API, which contains configured values which may be leveraged by other classes. To that end, I wonder if client or something along those lines wouldn't be a better variable name after an HfApi instantiation.
| @@ -13,7 +13,7 @@ def get_version() -> str: | |||
|
|
|||
| install_requires = [ | |||
| "filelock", | |||
| "requests", | |||
| "requests>=2.27", | |||
There was a problem hiding this comment.
Let's merge this with this pinned version, I'll open a PR today to fix it and remove the constraint before we release a new version. The pinned version is way too recent. cc @LysandreJik
|
Thanks for the work @SBrandeis. For future PRs, it'd be nice if you could send them from your own fork rather than creating feature branches on the main repo :) We're also trying to have a reviewer merge the PR rather than the author of the PR here :) |
|
OK, noted ! |
Closes #808