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

Extend import utils to cover "editable" torch versions #29000

Merged
merged 8 commits into from
Mar 15, 2024

Conversation

bhack
Copy link
Contributor

@bhack bhack commented Feb 13, 2024

What does this PR do?

Fixes #28999

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

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.

@bhack
Copy link
Contributor Author

bhack commented Feb 13, 2024

Why formatting error we have? it isn't clear from the CI log

@khipp
Copy link
Contributor

khipp commented Feb 13, 2024

The repository uses double quotes for string literals. You can format your code by running 'make style' (see contributor guideline).

@bhack
Copy link
Contributor Author

bhack commented Feb 14, 2024

Done, who can review this?

@amyeroberts
Copy link
Collaborator

@bhack Thanks for opening this PR!

For anyone who's coming to this PR in the future, could you share the torch version in your environment i.e. when running pip list | grep torch?

I've requested a review from @ydshieh as he is the king of all things version and package handling. I'd like to get his opinion before we merge. He's off for a for days, so would next week when this can get merged in.

@bhack
Copy link
Contributor Author

bhack commented Mar 7, 2024

Gently ping

@amyeroberts
Copy link
Collaborator

Gentle ping @ydshieh

@bhack Could you also share the torch version listed in your environment, as requested?

@bhack
Copy link
Contributor Author

bhack commented Mar 8, 2024

It is in the mentioned ticket in the description. This is for the devpytorch versions.

@amyeroberts
Copy link
Collaborator

@bhack Yes, I understand it's for the dev version, what I'm asking for is how that appears in your env when running pip list | grep torch

@bhack
Copy link
Contributor Author

bhack commented Mar 8, 2024

These are the nightly releases.. you can find the patterns at https://download.pytorch.org/whl/nightly/torch/

@ydshieh
Copy link
Collaborator

ydshieh commented Mar 14, 2024

Hi @bhack Thank you for the PR.

I am up to have this, but I am worried about the fact that the dev version might cause problem in the ecosystem.
For example, what would happen

version.parse(_torch_version)
and
version.parse(_torch_version) >= version.parse("2.1.1")

Would they fail? Would they give the correct results? ect.

@bhack
Copy link
Contributor Author

bhack commented Mar 14, 2024

Are we not working with this on dev only when the import fail?

@ydshieh
Copy link
Collaborator

ydshieh commented Mar 14, 2024

Well, I might lack some context, but my understanding is

if importlib.metadata.version(pkg_name) gives importlib.metadata.PackageNotFoundError (which happens with dev), we use importlib.import_module(pkg_name) to grab the version (and this means the import will work).

So dev could be imported, and we can grab its version. But it is unclear to me if the usages (that I listed in the previous comment) will work or give the correct results.

Hope the above make my concern a bit more clear

@bhack
Copy link
Contributor Author

bhack commented Mar 14, 2024

Yes but I don't understand your example. Where we are using this?

version.parse(_torch_version)
and
version.parse(_torch_version) >= version.parse("2.1.1")

@bhack
Copy link
Contributor Author

bhack commented Mar 14, 2024

If you meant here:

return version.parse(_torch_version) >= version.parse("2.1.1")

We can add the 'dev` version as it has the date as a fake version and control the date.

@bhack
Copy link
Contributor Author

bhack commented Mar 14, 2024

E.g just tested the PR code today and the assert is ok

``` 2.3.0.dev20240314`

import importlib
from importlib import metadata
package_exists = importlib.util.find_spec("torch") is not None
pkg_name="torch"
try:
    # Primary method to get the package version
    package_version = importlib.metadata.version(pkg_name)
    package_exists = True
except importlib.metadata.PackageNotFoundError:
    # Fallback method: Only for "torch" and versions containing "dev"
    if pkg_name == "torch":
        try:
            package = importlib.import_module(pkg_name)
            temp_version = getattr(package, "__version__", "N/A")
            # Check if the version contains "dev"
            if "dev" in temp_version:
                package_version = temp_version
                package_exists = True
            else:
                package_exists = False
        except ImportError:
            # If the package can't be imported, it's not available
            package_exists = False
    else:
        # For packages other than "torch", don't attempt the fallback and set as not available
        package_exists = False
print(f"Detected  version {package_version}")
print(f"Detected  version: {package_version}")
from packaging import version
assert(version.parse(package_version) >= version.parse("2.1.1"))

@ydshieh
Copy link
Collaborator

ydshieh commented Mar 14, 2024

Very nice! Thank you for checking, that's exactly what I am looking for.

@amyeroberts I tried to do the same (install torch editable version, but it took forever). The changes in this PR LGTM however.

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Thank you

@ydshieh
Copy link
Collaborator

ydshieh commented Mar 14, 2024

BTW, @bhack could you rebase the PR on the latest main branch? Thanks.

@bhack
Copy link
Contributor Author

bhack commented Mar 14, 2024

Done

@bhack
Copy link
Contributor Author

bhack commented Mar 14, 2024

I tried to do the same (install torch editable version, but it took forever). The changes in this PR LGTM however.

Strange, follow the official mentiond pytorch contributing markdown for rightly

It is quite fast to prepare.

@ydshieh
Copy link
Collaborator

ydshieh commented Mar 14, 2024

pytorch contributing

well, I was trying to use pip -e and found probably an old doc (in pytorch forum) that is probably not working anymore

@bhack
Copy link
Contributor Author

bhack commented Mar 14, 2024

No it is https://github.com/pytorch/pytorch/blob/main/CONTRIBUTING.md#nightly-checkout--pull

@HuggingFaceDocBuilderDev

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.

Co-authored-by: Yih-Dar <2521628+ydshieh@users.noreply.github.com>
@bhack bhack requested a review from ydshieh March 14, 2024 16:09
Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Just one comment

src/transformers/utils/import_utils.py Show resolved Hide resolved
@ydshieh
Copy link
Collaborator

ydshieh commented Mar 15, 2024

Still good to me, but we don't need the last commit Restore package_exists.

I will leave @amyeroberts to make the final decision however.

@bhack
Copy link
Contributor Author

bhack commented Mar 15, 2024

I think the same but it was explicitly requested. So, as you like.

@ydshieh
Copy link
Collaborator

ydshieh commented Mar 15, 2024

I think the same but it was explicitly requested. So, as you like.

Sure :-)

(but you can still express you thought instead of just following the request 😃 )

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for enabling this @bhack!

@amyeroberts amyeroberts merged commit f62407f into huggingface:main Mar 15, 2024
21 checks passed
itazap pushed a commit that referenced this pull request May 14, 2024
* Extend import utils to cover "editable" torch versions

* Re-add type hint

* Remove whitespaces

* Double quote strings

* Update comment

Co-authored-by: Yih-Dar <2521628+ydshieh@users.noreply.github.com>

* Restore package_exists

* Revert "Restore package_exists"

This reverts commit 66fd2cd.

---------

Co-authored-by: Yih-Dar <2521628+ydshieh@users.noreply.github.com>
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.

Pytorch not detected in official "editable" nightly
5 participants