Fix TPU tests on master builds #15349
Conversation
carmocca
added
ci
|
The changes here will not apply until they are merged. So this is ready. I'll check it after it's merged. This is fine because TPUs are not a required check |
| fi | ||
|
|
||
| echo "--- Install packages ---" | ||
| PACKAGE_NAME=lite pip install -e .[dev] |
There was a problem hiding this comment.
I stumbled upon something interesting regarding these two lines when I tried setting up a machine on Colab to run my TPU tests.
When you run PACKAGE_NAME=lite pip install -e .[dev], it will write _PACKAGE_NAME = "lite" in setup.py. This is done by these quite surprising lines of code: https://github.com/Lightning-AI/lightning/blob/master/setup.py#L72-L78
When you run PACKAGE_NAME=pytorch pip install -e .[dev] it installs once again lightning_lite because the PACKAGE_NAME variable is simply ignored.
You can see it in the logs of the run on the master branch:
- First install: https://github.com/Lightning-AI/lightning/actions/runs/3375739365/jobs/5602668694#step:10:128
- Second install: https://github.com/Lightning-AI/lightning/actions/runs/3375739365/jobs/5602668694#step:10:194
- You can also see that the
pytorch_lightningdoes not appear in the installed libraries that are listed withpip list: https://github.com/Lightning-AI/lightning/actions/runs/3375739365/jobs/5602668694#step:10:196
When running PACKAGE_NAME=lite pip install .[dev] without the -e option, it does not override the code in setup.py, so I could successfully set up the libraries like this.
Mysteriously, the tests that require pytorch_lightning run successfully, so it must be installed somehow. I presumed that it may come pre-installed in the docker image, but I couldn't deduce it from the commands given on the docker hub page.
There was a problem hiding this comment.
Yes, and I think it is rightthat the next env. variable is ignored because aside from the edit of setup.py also, MANIFEST.in is updated...
TLDR, I think we can drop -e from all CI 🦦
What does this PR do?
Jobs triggered from master do not have a PR number.
Also install both packages separately.
Does your PR introduce any breaking changes? If yes, please list them.
None
cc @carmocca @akihironitta @Borda @kaushikb11 @rohitgr7