Rethink conda tests in CI

[akihironitta]

I'm seeing a possible opportunity on deleting one of the workflows from our CI/CD. Specifically, I'm thinking to remove this workflow as I don't quite get why it exists.
https://github.com/Lightning-AI/lightning/blob/b25275ccc27652b91d85d49b7bc220b37c921b54/.github/workflows/ci-pytorch_test-conda.yml#L1

---

Questions

1. Why does the base-conda docker image have nvidia/cuda as its base image?
   https://github.com/Lightning-AI/lightning/blob/b25275ccc27652b91d85d49b7bc220b37c921b54/dockers/base-conda/Dockerfile#L17
   ce8abd6 added a Dockerfile for base-conda image for the first time, but I wasn't able to find any reasonable background for the change.

2. What is environment.yml for? I'm leaning towards removing the file because:

   • there's a discrepancy between environment.yml and requirements/pytorch/*.txt, and
   • only a few of optional dependencies are listed in the file.

3. Do we even need the conda CI workflow in the first place? I'm asking because it doesn't really test "conda" as most dependencies are installed via pip anyway (+ environment.yml is outdated as noted above).

---

Pitch

• Remove building base-conda docker images,
• remove ci-pytorch_test-conda.yml, and
• remove environment.yml.

---

Alternative

• Keep the conda job,
• make sure to keep environment.yml updated, and
• update the base image to something different than nvidia/cuda.

---

@carmocca @Borda Would love to hear your thoughts!

[carmocca]

I completely agree with this proposal. I asked the same questions in the past and was told that the conda job was added because at some point there was an issue with the conda installation. But I don't think that's enough reason for all this added complexity in our CI.

[Borda]

1. Why does the base-conda docker image have nvidia/cuda as its base image?

Back then when it was created I guess nvidia/cuda did not exist

2. What is environment.yml for?

it is needed for Conda installation from the source

3. Do we even need the conda CI workflow in the first place?

I think yes as it is testing particular PT versions as some compatibility could be different and you also see in our codebase that we have sometimes patches related to specific PT versions...

[akihironitta]

1. Why does the base-conda docker image have nvidia/cuda as its base image?

Back then when it was created I guess nvidia/cuda did not exist

I might not get what you mean... so there's basically no reason to use nvidia/cuda that base-conda is currently based on for conda CPU CI job now then?

2. What is environment.yml for?

it is needed for Conda installation from the source

I see. If it's valuable to anyone, we can keep the file in the repo making sure it's always updated, but my question is "does anyone actually use it (outside our CI)?"

Also, regarding the validity of conda installation, there seems to be a test that validates import pytorch_lightning with dependencies installed with conda in the conda-forge feedstock repo, so the conda installation can always be validated there even if we get rid of this file from our repo, I think.
https://github.com/conda-forge/pytorch-lightning-feedstock/blob/afa2967b67f1576eafe7b8cce462d5916952c62f/recipe/meta.yaml#L17-L38

3. Do we even need the conda CI workflow in the first place?

I think yes as it is testing particular PT versions as some compatibility could be different and you also see in our codebase that we have sometimes patches related to specific PT versions...

If we test all PyTorch versions in the "full" testing job, it's possible to remove the conda workflow without any concerns on compatibility, I think?

[akihironitta]

Using Conda was there on purpose as in past, we had reported issues that users with Conda had some problems, so at least keep Conda for push to master...
#14387 (review)

Given @Borda's suggestion, do we agree that we should keep the conda job instead of removing it completely as being addressed in #14387?

I'm in favour of removing the conda job to simplify our CI.

cc @carmocca for visibility

[carmocca]

I am too. If we wanted to keep it, it should just try to import the package, not run tests. But then we already have that in conda-forge.

In the time I've been in this repository, I've never seen a tests issue caused by downloading from conda

[awaelchli]

Not sure if this is relevant or known, but we sometimes see conda jobs failing while the other ones pass. So they must be executing a different set of tests.

Example:

Conda tests: https://github.com/Lightning-AI/lightning/actions/runs/3056881005/jobs/4931459434
Others: https://github.com/Lightning-AI/lightning/actions/runs/3056881003/jobs/4931464736

Perhaps we should first figure out this mystery before removing them :)

[carmocca]

Removing these separate testing workflows should make it easier to see what's the difference when one job fails but the rest pass. As the entire matrix of Python and PyTorch would be launched from the same workflow

[awaelchli]

Perhaps we should still keep a small job that at least installs the conda version of our package, and test the import. There seem to be issues: #14765, #14374

[carmocca]

This was also suggested in

Also, regarding the validity of conda installation, there seems to be a test that validates import pytorch_lightning with dependencies installed with conda in the conda-forge feedstock repo, so the conda installation can always be validated there even if we get rid of this file from our repo, I think.
https://github.com/conda-forge/pytorch-lightning-feedstock/blob/afa2967b67f1576eafe7b8cce462d5916952c62f/recipe/meta.yaml#L17-L38

and

If we wanted to keep it, it should just try to import the package, not run tests. But then we already have that in conda-forge.

The fact that our CI doesn't raise these issues favors the argument that having a separate testing configuration for Conda is not useful as it is now.

[Borda]

Ok, let's use just pip, but add another workflow for testing installation with Conda...

[akihironitta]

Sounds good to me! For reference, #14374 is the tracking issue for adding a conda installation test, and #14387 is for unifying PL tests into a single workflow.