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

CI fixes for conda forge #29

Merged
merged 17 commits into from
Oct 19, 2023
Merged

CI fixes for conda forge #29

merged 17 commits into from
Oct 19, 2023

Conversation

hmacdope
Copy link
Contributor

Fixes #28

@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2023

Codecov Report

Merging #29 (c6f3c04) into main (07ea82b) will increase coverage by 0.33%.
Report is 9 commits behind head on main.
The diff coverage is 32.14%.

Additional details and impacted files

@hmacdope hmacdope changed the title Ci and tests CI fixes Oct 17, 2023
@hmacdope
Copy link
Contributor Author

@mikemhenry @kaminow I think the failure to solve for 3.9 and 3.11 is due to BLAS restrictions in our conda recipie for DGL, pushing a hopeful fix now conda-forge/dgl-feedstock#20

@hmacdope
Copy link
Contributor Author

@mikemhenry would you mind having a look here at the conflict graph, as far as I can tell there are 3.9 has a bunch of issues and 3.11 is blocked by some pytorch-cluster issues.

@hmacdope
Copy link
Contributor Author

hmacdope commented Oct 18, 2023

If we can't get to the bottom of this, then we may have to use the dglteam channel in the short term. @kaminow

@hmacdope hmacdope changed the title CI fixes CI fixes for conda forge Oct 18, 2023
@ijpulidos
Copy link

Interestingly enough, I can create an environment fine with mamba (not micromamba) if I use the following env file (removed the dglteam channel and added python=3.9).

name: test
channels:
  - conda-forge
dependencies:
  - python=3.9
  - pytorch
  - pytorch_geometric
  - pytorch_cluster
  - pytorch_scatter
  - pytorch_sparse
  - numpy
  - h5py
  - e3nn
  - dgllife
  - dgl
  - rdkit
  # testing dependencies
  - pytest
  - pytest-cov
  - codecov

and running mamba env create -f devtools/conda-envs/test_env.yaml -n mtenn-test

@mikemhenry
Copy link
Contributor

If we can't get to the bottom of this, then we may have to use the dglteam channel in the short term. @kaminow

Looking into this, but IMHO, way better to just drop python 3.9 support (or not test it on CI since it sounds like it is only a problem on CI, since @ijpulidos can make an env and so can I, micromamba create -n mtenn-py39 --dry-run --file devtools/conda-envs/test_env.yaml python=3.9 works for me on on Linux)

@mikemhenry
Copy link
Contributor

CONDA_OVERRIDE_CUDA="" micromamba create -n mtenn-py39 --dry-run --file devtools/conda-envs/test_env.yaml python=3.9 works for me as well, thought I should check that to make sure it wasn't a GPU thing

@mikemhenry
Copy link
Contributor

mikemhenry commented Oct 19, 2023

So my vote is to not worry about this unless we need python 3.9 in a production environment and drop python 3.9 in our testing matrix.

@kaminow
Copy link
Collaborator

kaminow commented Oct 19, 2023

I think the main worry with ignoring the issues with 3.9 is that it may mess with the asapdiscovery CI for 3.9, so we'd have to drop it there as well

@mikemhenry
Copy link
Contributor

It looks like we have mtenn in our CI already there and it is working https://github.com/choderalab/asapdiscovery/blob/main/devtools/conda-envs/asapdiscovery-ubuntu-latest.yml#L90 so we wouldn't have to drop 3.9 from our CI matrix for asapdiscovery

Copy link
Collaborator

@kaminow kaminow left a comment

Choose a reason for hiding this comment

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

going to approve this based on our discussion today

@kaminow kaminow merged commit 54c94b0 into choderalab:main Oct 19, 2023
4 checks passed
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.

Add tests and CI
5 participants