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

Add plus notation to wheel filenames at data.pyg.org/whl #66

Closed
ddelange opened this issue Jul 25, 2022 · 36 comments
Closed

Add plus notation to wheel filenames at data.pyg.org/whl #66

ddelange opened this issue Jul 25, 2022 · 36 comments

Comments

@ddelange
Copy link

ddelange commented Jul 25, 2022

🐛 Describe the bug

it would be cool to introduce the plus notation in the wheel filenames, to avoid having to pass --force-reinstall

currently, the filenames are identical (just a different URL path), which confuses pip into thinking there is no need for a re-install:

https://data.pyg.org/whl/torch-1.9.0+cu111/torch_sparse-0.6.12-cp36-cp36m-linux_x86_64.whl
https://data.pyg.org/whl/torch-1.9.0+cpu/torch_sparse-0.6.12-cp36-cp36m-linux_x86_64.whl
# vs
https://download.pytorch.org/whl/cu111/torch-1.9.0+cu111-cp36-cp36m-linux_x86_64.whl
https://download.pytorch.org/whl/cpu/torch-1.9.0+cpu-cp36-cp36m-linux_x86_64.whl

use-case: docker image (and other CI pipelines) building on top of env that already contains the cpu versions

ref ray-project/ray#26072 (comment)

current behaviour (whereas torch would be overwritten at the second command due to plus notation in the wheel filenames):

$ pip install --no-deps torch-sparse==0.6.12 --find-links https://data.pyg.org/whl/torch-1.9.0+cpu.html
Looking in links: https://data.pyg.org/whl/torch-1.9.0+cpu.html
Collecting torch-sparse==0.6.12
  Using cached https://data.pyg.org/whl/torch-1.9.0%2Bcpu/torch_sparse-0.6.12-cp38-cp38-linux_x86_64.whl (640 kB)
Installing collected packages: torch-sparse
Successfully installed torch-sparse-0.6.12
$ pip install --no-deps torch-sparse==0.6.12 --find-links https://data.pyg.org/whl/torch-1.9.0+cu111.html
Looking in links: https://data.pyg.org/whl/torch-1.9.0+cu111.html
Requirement already satisfied: torch-sparse==0.6.12 in /usr/local/lib/python3.8/dist-packages (0.6.12)
$ pip install --no-deps --force-reinstall torch-sparse==0.6.12 --find-links https://data.pyg.org/whl/torch-1.9.0+cu111.html
Looking in links: https://data.pyg.org/whl/torch-1.9.0+cu111.html
Collecting torch-sparse==0.6.12
  Downloading https://data.pyg.org/whl/torch-1.9.0%2Bcu111/torch_sparse-0.6.12-cp38-cp38-linux_x86_64.whl (3.7 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 3.7/3.7 MB 4.0 MB/s eta 0:00:00
Installing collected packages: torch-sparse
  Attempting uninstall: torch-sparse
    Found existing installation: torch-sparse 0.6.12
    Uninstalling torch-sparse-0.6.12:
      Successfully uninstalled torch-sparse-0.6.12
Successfully installed torch-sparse-0.6.12

--no-deps only added for demo purposes (shorter logs)

plus notation is a non-breaking change from pip's side (current install commands will still work identically), it will just allow pip to differentiate the use-case above.

Environment

N/A

@rusty1s
Copy link
Member

rusty1s commented Jul 26, 2022

I agree. We should add this in ASAP to match PyTorch behavior. One downside though is that we can only resolve CUDA versions with this, not PyTorch versions.

@ddelange
Copy link
Author

ddelange commented Jul 26, 2022

One downside though is that we can only resolve CUDA versions with this, not PyTorch versions.

Yep, I guess you would have to add +cpu-pt012 or so to the wheel filenames 🤔

There is also https://github.com/chriskuehl/dumb-pypi which would still allow hosting static files, but with --extra-index-url instead of --find-links. Then you could have e.g.

$ PIP_EXTRA_INDEX_URL=https://data.pyg.org/whl/torch-1.9.0+cpu,https://download.pytorch.org/whl/cpu \
  pip install torch==1.9.0 torch-sparse ...

analogous to https://download.pytorch.org/whl/cpu.

Say you don't encode pytorch version in the plus notation, it would be cool to investigate whether index urls can help overcome that problem (find-links wouldn't):

$ pip install torch-sparse --extra-index-url https://data.pyg.org/whl/torch-1.9.0+cu111
...
$ pip install torch-sparse --extra-index-url https://data.pyg.org/whl/torch-1.10.0+cu111
hope that pip triggers a re-install despite identical wheel filename due to different index url

@ddelange
Copy link
Author

ddelange commented Jul 27, 2022

On second thought, I think the PIP_INDEX_URL setup from my previous comment is not necessary. The current setup with PIP_FIND_LINKS will work also to encode pytorch versions as long as e.g. PIP_FIND_LINKS=https://data.pyg.org/whl/torch-1.9.0+cpu.html serves only wheel filenames containing +torch190-cpu (or any other variation, as long as the plus suffix is unique & exclusive to each PIP_FIND_LINKS, and only contains [a-z0-9-]).

You can then simply

export PIP_EXTRA_INDEX_URL=https://download.pytorch.org/whl/cpu
export PIP_FIND_LINKS=https://data.pyg.org/whl/torch-1.9.0+cpu.html
pip install torch==1.9.0 torch-sparse ...

@ddelange
Copy link
Author

@rusty1s if the last comment looks good to you, and you can point me to the relevant code, I'm happy to PR next week!

@rusty1s
Copy link
Member

rusty1s commented Jul 28, 2022

This would be awesome. I think the fix is rather trivial as all we need to do is customize the name when building nightly wheels - see here. We already do this to inject the current date into the wheel name.

@ddelange
Copy link
Author

I think the fix is rather trivial as all we need to do is customize the name when building nightly wheels

Sounds good, not sure where stable wheels are pushed to s3, so I would then leave it to you to do it if that's ok 👍

@rusty1s
Copy link
Member

rusty1s commented Jul 28, 2022

Sounds good. Thank you!

@ddelange
Copy link
Author

hi @rusty1s 👋 is there any chance your team could reserve some resources for this fix?

cc @amogkam from ray-project/ray#26072

@rusty1s
Copy link
Member

rusty1s commented Aug 29, 2022

Hi @ddelange, I thought you wanted to start this, sorry for the misunderstanding. Do you need this for torch-scatter and torch-sparse as well?

@ddelange
Copy link
Author

No worries! I think relevant packages (from PR linked above) are torch-scatter, torch-sparse, torch-cluster, torch-spline-conv

@ddelange
Copy link
Author

ddelange commented Aug 29, 2022

Sorry if I communicated badly before. It's just that I wouldn't know where to make changes affecting the URLs (filenames) of the production wheels.

Concerning the scope of this issue: I think all wheels that are cuda/torch specific are affected, so all projects except torch-geometric.

@ddelange
Copy link
Author

Hi @rusty1s 👋

Any update on an ETA?

I tried again to find the code where production wheels get their filenames (built for different torch versions), but could only find the nightly setup you linked to before, which are not the wheels with the stable release __version__ inside.

I would be happy to push a fix for this (more or less trivial) issue, I just don't know where 😅

@rusty1s
Copy link
Member

rusty1s commented Oct 28, 2022

We will include this as part of our first pyg-lib release. Since we deperecate torch-scatter and torch-sparse soon, this is not top prio, so appreciate any help on this one. We build the wheels here, and would need to follow a similar procedure as in pyg-lib to change the naming.

ddelange added a commit to ddelange/pytorch_sparse that referenced this issue Oct 28, 2022
@ddelange
Copy link
Author

ddelange commented Oct 28, 2022

No worries! I think relevant packages (from PR linked above) are torch-scatter, torch-sparse, torch-cluster, torch-spline-conv

Thanks for the reply, I opened four identical PRs to change filename (so not internal __version__ like with nightlies) analogous to torch, before uploading to s3.

Like discussed above, it encodes e.g. +torch-1.12.1-cu116, so users switching torch versions or switching eg cpu to gpu find-links should trigger a re-install.

If anything else, please ping! As for me, with these PRs in place and new patch releases out, this issue can be closed 💪

@ddelange
Copy link
Author

We will include this as part of our first pyg-lib release.

Ah just re-read, in that case you may want to keep this issue open for upcoming pyg-lib specifically 👍

@rusty1s
Copy link
Member

rusty1s commented Oct 28, 2022

Thanks! I will try to merge tomorrow.

@ddelange
Copy link
Author

ddelange commented Nov 8, 2022

hi @rusty1s 👋

any chance these PRs could get a merge & release to unblock downstream?

@rusty1s
Copy link
Member

rusty1s commented Nov 8, 2022

Yes, I am currently building for PyTorch 1.13 but have a few problems with conda builds. This should be integrated once done.

@rusty1s
Copy link
Member

rusty1s commented Nov 14, 2022

Hey @ddelange. I am onto it. As far as I understand, renaming the wheels is sufficient, is that correct? Does the URL needs to be changed as well or just the name in the *.html files?

@ddelange
Copy link
Author

ddelange commented Nov 14, 2022

the link that's passed to --find-links can be an arbitrary URL, there is nothing parsed out of that URL.

for the actual content on that page, it looks to me like only the href attribute of the <a>'s needs to contain the plus-notation, but I can't 100% confirm that pip doesn't consider the link's display text.

to be sure (as well as to avoid human confusion), it probably makes sense that the href attribute corresponds to the display text of the link.

@rusty1s
Copy link
Member

rusty1s commented Nov 21, 2022

I uploaded and renamed all wheels to https://data.pyg.org/wheels, see, e.g., https://data.pyg.org/wheels/torch-1.13.0/cpu/. However, it does not seem to work yet :(

pip install torch_spline_conv --no-index --extra-index-url https://data.pyg.org/wheels/torch-1.13.0/cpu

@ddelange
Copy link
Author

  • I think I made a mistake: most likely, - and . are not allowed in the plus-string. pytorch has only +cpu or +cu111 etc.
  • It looks like you switched the wheel hosting from a --find-links structure (flat links on a html page) to --extra-index-url structure (typo in your comment above btw). That's fine, actually better, and closes PEP 503-standard repo URL pytorch_geometric#5625.

Just removing - and . from the plus-string in the wheel filenames should fix it!

@rusty1s
Copy link
Member

rusty1s commented Nov 21, 2022

Mh, it still downloads the package from https://pypi.org/simple rather than https://data.pyg.org/wheels/torch-1.13.0/cpu
:(

pip install torch_spline_conv --no-cache-dir --verbose --extra-index-url https://data.pyg.org/wheels/torch-1.13.0/cpu
Using pip 22.3.1 from /data-1T-vol/users/matthias/miniconda3/lib/python3.9/site-packages/pip (python 3.9)
Looking in indexes: https://pypi.org/simple, https://data.pyg.org/wheels/torch-1.13.0/cpu
Collecting torch_spline_conv
  Downloading torch_spline_conv-1.2.1.tar.gz (13 kB)
  Running command python setup.py egg_info

@ddelange
Copy link
Author

i currently see torch_sparse-0.6.15+torch-1.13.0-cpu-cp310-cp310-linux_x86_64.whl, i think pip fails to parse it because of +torch-1.13.0, i think you need eg +torch1130cu116 in the links

@ddelange
Copy link
Author

with both torch and cuda versions encoded in the filenames, when switching extra index url, pip will read new filenames due to thay switch, and trigger a re-install. so both torch and cuda are needed in the filename so that it works as expected when bumping either/or/both.

@rusty1s
Copy link
Member

rusty1s commented Nov 21, 2022

Yeah, I just changed it for a single wheel, see https://data.pyg.org/wheels/torch-1.13.0/cpu/torch_spline_conv/ (+pt113cpu). Need to do some investigation why this file doesn't get picked up.

@ddelange
Copy link
Author

ddelange commented Nov 21, 2022

does it work when you pip install torch_spline_conf --no-index --find-links https://data.pyg.org/wheels/torch-1.13.0/cpu/torch_spline_conv/ ?

if yes, there might be some magic concerning how index urls work, missing metadata on the html or something (vs the find-links mechanic which just searches for <a> on the page)

@rusty1s
Copy link
Member

rusty1s commented Nov 21, 2022

This is a good point. It fails due to inconsistent metadata :(

Looking in links: https://data.pyg.org/wheels/torch-1.13.0/cpu/torch_spline_conv/
Collecting torch_spline_conv
  Downloading https://data.pyg.org/wheels/torch-1.13.0/cpu/torch_spline_conv/torch_spline_conv-1.2.1%2Bpt113cpu-cp39-cp39-linux_x86_64.whl (203 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 203.2/203.2 kB 397.4 kB/s eta 0:00:00
Discarding https://data.pyg.org/wheels/torch-1.13.0/cpu/torch_spline_conv/torch_spline_conv-1.2.1%2Bpt113cpu-cp39-cp39-linux_x86_64.whl (from https://data.pyg.org/wheels/torch-1.13.0/cpu/torch_spline_conv/): Requested torch_spline_conv from https://data.pyg.org/wheels/torch-1.13.0/cpu/torch_spline_conv/torch_spline_conv-1.2.1%2Bpt113cpu-cp39-cp39-linux_x86_64.whl has inconsistent version: expected '1.2.1+pt113cpu', but metadata has '1.2.1'
ERROR: Could not find a version that satisfies the requirement torch_spline_conv (from versions: 1.2.1+pt113cpu)
ERROR: No matching distribution found for torch_spline_conv

Guess I have to re-build everything :( :(

@ddelange
Copy link
Author

I think you're right ;(
image

@rusty1s
Copy link
Member

rusty1s commented Nov 23, 2022

Yeah, I will try to update the wheels for PyTorch 1.13.

@rusty1s
Copy link
Member

rusty1s commented Nov 24, 2022

I just pushed this change for torch-scatter==2.1.0, see here. It has the suffix +pt113cu116. Let me know if this is sufficient for you :)

@ddelange
Copy link
Author

that looks perfect! this will allow to do the same as is possible with torch / torchaudio etc 💪 could you do the same for torch-sparse, torch-cluster, torch-spline-conv?

@rusty1s
Copy link
Member

rusty1s commented Nov 25, 2022

All done. I guess we can finally close this issue. Thanks for your patience!

@rusty1s rusty1s closed this as completed Nov 25, 2022
@ddelange
Copy link
Author

Thanks for your persistence! 💥

@alexandervaneck
Copy link

@rusty1s @ddelange Thank you for looking into this. It's certainly an improvement over the previous situation! 🙇

I believe to have found an edge case which resurfaces the original comment of "currently, the filenames are identical (just a different URL path), which confuses pip into thinking there is no need for a re-install"

The torch version in the + metadata is specified up to the minor version and therefore filenames can be identical between torch versions.

In https://data.pyg.org/whl/torch-1.13.1%2Bcu116.html (for 1.13.1)
pyg_lib-0.1.0+pt113cu116-cp310-cp310-linux_x86_64.whl
In https://data.pyg.org/whl/torch-1.13.0%2Bcu116.html (for 1.13.0)
pyg_lib-0.1.0+pt113cu116-cp310-cp310-linux_x86_64.whl

This then resurfaces all the mentioned 'problems' with pip not reinstalling when it clearly should when the torch version (or cuda) version is updated. Could we fix that by setting the torch version to the patch version?

@rusty1s
Copy link
Member

rusty1s commented Feb 17, 2023

Thanks for reporting. There is no need to re-install for patch versions. Am I missing something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants