Eager pip install #13198

carmocca · 2022-06-01T10:41:35Z

What does this PR do?

Fixes the problem described in #13048 (comment)

Does your PR introduce any breaking changes? If yes, please list them.

None

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
[n/a] Did you make sure to update the documentation with your changes? (if necessary)
[n/a] Did you write any new necessary tests? (not for typos and docs)
[n/a] Did you verify new and existing tests pass locally with your changes?
Did you list all the breaking changes introduced by this pull request?
[n/a] Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

cc @carmocca @akihironitta @Borda @tchaton @rohitgr7

.github/workflows/docs-checks.yml

Borda

seem complete for me

carmocca · 2022-06-02T11:21:46Z

.github/workflows/ci_test-full.yml

-        pip install -r requirements/examples.txt --find-links https://download.pytorch.org/whl/cpu/torch_stable.html --upgrade
-        pip install -r requirements/test.txt --upgrade
+        pip install -r requirements/examples.txt --find-links https://download.pytorch.org/whl/cpu/torch_stable.html -U --upgrade-strategy eager
+        pip install -r requirements/test.txt -U --upgrade-strategy eager


CI is failing because a test.txt dependency requires protobuf and since we are marking eager here, it gets upgraded to the latest version since it's not considering the pin in requirements.txt

Not sure about how we can resolve this problem. All requirements would need to be installed together instead of in steps so the resolution algorithm can find a working combination.

carmocca · 2022-06-28T15:45:48Z

Closing, as I don't think we can fix the issue this way reliably.

I think the best option now would be to have some sort of lock file by using a tool like poetry.

Borda · 2022-06-29T12:45:26Z

@carmocca re-opened and set the eager mode only for the one CI which shall be running the latest versions... I think that is the best place we shall be testing compatibility 🐰

carmocca · 2022-06-29T12:56:13Z

But it needs to be done for all requirements, not just PyTorch. In fact, last breakages were from dependabot upgrading DeepSpeed and NumPy.

I don't see how it solves the issue described in #13198 (comment)

Borda · 2022-06-29T13:09:30Z

fyi, shall be passing after #13441

Borda · 2022-06-29T13:11:43Z

I don't see how it solves the issue described in #13198 (comment)

yes, this won't address installing requirements together but set the proper upgrade for testing the bumped version are correct/feasible so let's do it step by step...
merge this and check how to join and install the remaining req. or do you prefer to do it single PR?

carmocca · 2022-06-29T15:13:27Z

merge this and check how to join and install the remaining req. or do you prefer to do it single PR?

I would prefer to have a general solution that works for all requirements before starting to make changes. This will make any possible issues pop up

Borda · 2022-06-29T17:07:16Z

I would prefer to have a general solution that works for all requirements before starting to make changes.

but be aware that no other CI/workflow requires running the latest, other rather uses cache, for example, Conda has specified PT versions and other like dockers are fine to build with a version available not necessary the latest... so I believe we shall just update this CI job

Borda · 2022-06-29T17:08:41Z

.github/workflows/ci-pytorch_test-full.yml

@@ -143,7 +147,7 @@ jobs:
      run: |
        # adjust versions according installed Torch version
        python ./requirements/pytorch/adjust-versions.py requirements/pytorch/examples.txt
-        pip install -r requirements/pytorch/examples.txt --find-links https://download.pytorch.org/whl/cpu/torch_stable.html --upgrade
+        pip install $PIP_FLAG -r requirements/pytorch/examples.txt --find-links "$TORCH_URL --upgrade


so what we can do here is reinstall the all above to reduce installing different versions to be not sliding with example requirements...

carmocca · 2022-06-29T17:32:48Z

The point of this PR was that all installations would use the newest available version.

For "oldest" jobs, there's nothing to do because they are pinned already to the min version: ==
For "latest" jobs, caches mess things up because we don't pin (==) to the max version. For this reason, the version in requirements can be updated when in reality it will fail overnight. Two examples are:

NumPy update: Fix mypy errors for model summary utilities #13384 caused by Update numpy requirement from <=1.22.3,>=1.17.2 to >=1.17.2,<1.22.5 in /requirements #13361
DeepSpeed update: Pin protobuf<=3.20.1 #13177 caused by Update deepspeed requirement from <0.6.0 to <0.7.0 in /requirements #13048

What this PR tried to do is to always force eager ugprades. But this needs to be done for all dependencies. However issues arise from installing different dependencies in different steps since the resolution algorithm does not consider the requirements from a previous step.
It's not just a problem with PyTorch, but every single package.

So I don't see how the changes in the PR now help.

Another option would be to do the max version pinning just as we do for min version pinning.

stale · 2022-07-14T03:43:43Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/generated/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

stale · 2022-07-22T07:29:09Z

This pull request is going to be closed. Please feel free to reopen it create a new from the actual master.

carmocca added the ci Continuous Integration label Jun 1, 2022

carmocca added this to the 1.6.x milestone Jun 1, 2022

carmocca self-assigned this Jun 1, 2022

carmocca commented Jun 1, 2022

View reviewed changes

.github/workflows/docs-checks.yml Outdated Show resolved Hide resolved

Borda approved these changes Jun 2, 2022

View reviewed changes

carmocca force-pushed the ci/upgrade-eager branch from 21cc7b4 to c2435ee Compare June 2, 2022 09:55

carmocca marked this pull request as ready for review June 2, 2022 09:55

carmocca requested a review from tchaton as a code owner June 2, 2022 09:55

carmocca commented Jun 2, 2022

View reviewed changes

mergify bot added has conflicts and removed has conflicts labels Jun 2, 2022

akihironitta mentioned this pull request Jun 22, 2022

Update deepspeed requirement from <0.6.0 to <0.7.0 in /requirements #13367

Closed

carmocca closed this Jun 28, 2022

carmocca deleted the ci/upgrade-eager branch June 28, 2022 15:45

Borda restored the ci/upgrade-eager branch June 29, 2022 12:35

Borda reopened this Jun 29, 2022

carmocca removed their assignment Jun 29, 2022

carmocca removed this from the pl:1.6.x milestone Jun 29, 2022

full eager

679267e

Borda force-pushed the ci/upgrade-eager branch from 6e45d19 to 679267e Compare June 29, 2022 12:43

mergify bot removed the has conflicts label Jun 29, 2022

Borda requested review from akihironitta and a team June 29, 2022 12:44

simple

f95fcc0

Borda added the priority: 1 Medium priority task label Jun 29, 2022

typo

feb709b

Borda added 2 commits June 29, 2022 14:57

...

d38ba0a

win

ae619b4

Merge branch 'master' into ci/upgrade-eager

d539e1c

Borda reviewed Jun 29, 2022

View reviewed changes

stale bot added the won't fix This will not be worked on label Jul 14, 2022

mergify bot added the has conflicts label Jul 15, 2022

stale bot closed this Jul 22, 2022

carmocca deleted the ci/upgrade-eager branch July 22, 2022 13:11

carmocca mentioned this pull request Feb 1, 2023

Bump max deepspeed version to 0.8.0 #16469

Merged

12 tasks

carmocca self-assigned this Nov 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eager pip install #13198

Eager pip install #13198

carmocca commented Jun 1, 2022 •

edited by github-actions bot

Loading

Borda left a comment

carmocca Jun 2, 2022

carmocca commented Jun 28, 2022

Borda commented Jun 29, 2022

carmocca commented Jun 29, 2022

Borda commented Jun 29, 2022

Borda commented Jun 29, 2022 •

edited

Loading

carmocca commented Jun 29, 2022

Borda commented Jun 29, 2022

Borda Jun 29, 2022

carmocca commented Jun 29, 2022

stale bot commented Jul 14, 2022

stale bot commented Jul 22, 2022

Eager pip install #13198

Eager pip install #13198

Conversation

carmocca commented Jun 1, 2022 • edited by github-actions bot Loading

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

PR review

Borda left a comment

Choose a reason for hiding this comment

carmocca Jun 2, 2022

Choose a reason for hiding this comment

carmocca commented Jun 28, 2022

Borda commented Jun 29, 2022

carmocca commented Jun 29, 2022

Borda commented Jun 29, 2022

Borda commented Jun 29, 2022 • edited Loading

carmocca commented Jun 29, 2022

Borda commented Jun 29, 2022

Borda Jun 29, 2022

Choose a reason for hiding this comment

carmocca commented Jun 29, 2022

stale bot commented Jul 14, 2022

stale bot commented Jul 22, 2022

carmocca commented Jun 1, 2022 •

edited by github-actions bot

Loading

Borda commented Jun 29, 2022 •

edited

Loading