simplify get_class_in_module and fix for paths containing a dot #29262
Conversation
cebtenzzre
force-pushed
the
fix-dot-in-paths
branch
from
72fd557 to
009b504
Compare
There was a problem hiding this comment.
Strong approval!
For @ArthurZucker: This PR is fixing the same problem as my PR #29175. However, it has two advantages. Firstly, it's cleaner, rather than a messy try-except, and secondly it's more universal and doesn't fail in other cases when . is present in the repo path.
The only thing I'm confused about is that @cebtenzzre said that the test I added in #29175 was still passing even before that PR. That's strange to me - my test explicitly called model = AutoModel.from_pretrained("hf-internal-testing/test_dynamic_model_v1.0", trust_remote_code=True), which should definitely have failed before my PR because the repo name contains .
I'm not sure what's going on there - I'd appreciate any clarification from @cebtenzzre about what he thinks the cause is! Ideally we'd make sure that test is working correctly before we merge this.
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
Update: @not-lain explained the issue with the test. The repo being tested has now been updated, so that the test actually tests what we wanted to test in the first place, and that resolves my question! @cebtenzzre I don't have any other objections, so we just need core maintainer review to merge this! |
| """ | ||
| Import a module on the cache directory for modules and extract a class from it. | ||
|
|
||
| Args: | ||
| repo_id (`str`): The repo containing the module. Used for path manipulation. | ||
| class_name (`str`): The name of the class to import. | ||
| module_path (`str` or `os.PathLike`): The path to the module to import. |
There was a problem hiding this comment.
module_path now expects the file extension too now. Maybe that should be reflected here too? @cebtenzzre
What does this PR do?
A simpler implementation of get_class_in_module that also works when the path starts with e.g.
./or otherwise contains dot characters outside of the repo ID.Fixes #29251
I should write a test. The test in #29175 didn't fail before the PR (with code from commit 89c6481), so it does not seem valid.
This does change the API of get_class_in_module to expect the path to an actual file, with the extension - but since the previous PR changed the API already, this seems fine.
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.