Conversation
* Handle changes in scipy.logsumexp behaviour * Add CMMMotion remover when creating system from xml to match change in behaviour in Interchange
I've gone ahead and done this, just to check the CI runs. |
Yoshanuikabundi
left a comment
There was a problem hiding this comment.
Looks good! I have a few nits on how the environments are set up - would be good to improve the experience for developers who don't have a Linux/CUDA machine, which is I think most of the people who are likely to work on this. Unfortunately Pixi makes it a bit difficult to work with PyTorch from custom PyPi indexes, so we're limited in how accessible that is for now.
Comments marked "(non-blocking)" are improvements that I think will help most other devs working on this project, but if you judge that they will impair your work more than they will help everyone else that's your call. Comments marked "(blocking)" are the bare minimum for things to work on certain other machines, so I think they're important.
Hope you're enjoying Pixi!
pyproject.toml
Outdated
| default = { features = ["cu12", "mm", "fe", "examples", "dev", "docs", "pydantic-2"] } | ||
| cpu = { features = ["cpu", "mm", "fe", "examples", "dev", "docs", "pydantic-2"] } | ||
| # The fe feature is excluded when pydantic < 2 as femto (used by absolv) needs pydantic >= 2 | ||
| pydantic-1-cu12 = { features = ["cu12", "mm", "examples", "dev", "docs", "pydantic-1"] } | ||
| pydantic-1-cpu = { features = ["cpu", "mm", "examples", "dev", "docs", "pydantic-1"] } |
There was a problem hiding this comment.
(non-blocking) Using solve groups makes sure that you're using the same versions of shared packages across different environments. This makes debugging upstream issues easier to collaborate on. The default is for each environment to be solved independently:
| default = { features = ["cu12", "mm", "fe", "examples", "dev", "docs", "pydantic-2"] } | |
| cpu = { features = ["cpu", "mm", "fe", "examples", "dev", "docs", "pydantic-2"] } | |
| # The fe feature is excluded when pydantic < 2 as femto (used by absolv) needs pydantic >= 2 | |
| pydantic-1-cu12 = { features = ["cu12", "mm", "examples", "dev", "docs", "pydantic-1"] } | |
| pydantic-1-cpu = { features = ["cpu", "mm", "examples", "dev", "docs", "pydantic-1"] } | |
| default = { features = ["cu12", "mm", "fe", "examples", "dev", "docs", "pydantic-2"], solve-group = "default" } | |
| cpu = { features = ["cpu", "mm", "fe", "examples", "dev", "docs", "pydantic-2"], solve-group = "default" } | |
| # The fe feature is excluded when pydantic < 2 as femto (used by absolv) needs pydantic >= 2 | |
| pydantic-1-cu12 = { features = ["cu12", "mm", "examples", "dev", "docs", "pydantic-1"], solve-group = "pydantic-1" } | |
| pydantic-1-cpu = { features = ["cpu", "mm", "examples", "dev", "docs", "pydantic-1"], solve-group = "pydantic-1" } |
There was a problem hiding this comment.
Thanks, I tried this, but ended up going back to separate solve groups to allow the pytorch version to be different between the cpu and cuda environments.
There was a problem hiding this comment.
Ahh yeah that makes sense, good call!
This reverts commit 1c6e380.
This will be removed in openforcefield#140
This reverts commit e8c1b05.
This reverts commit 5d6a420.
This reverts commit 51659a5.
|
Thanks for the very helpful review! And yes, I love I think I've addressed your points other than adding |
|
For me it's +1 on We quietly dropped support for this elsewhere and, at least for a first pass, I don't think it's worth worrying about here |
|
Matt's take makes complete sense to me! |
|
Thanks @fjclark, please merge when you're ready -- just added you to write access :-) |
|
Thanks @lilyminium! |
Description
Switches to
pixifor package management. This fixes issues such as the one described here avoiding duplication of shared dependencies in conda environment yaml files and also makes the Makefile redundant as we can specific commands forpixiinpyproject.toml. I've also updated the CI and documentation. All tests pass for pydantic > and < 1 environments once merged with #129 (but will fail as-is).Happy to merge this and #129 into a single PR with passing CI if that's preferable. Thanks!
Status