search

LLNL / merlin

Machine Learning for HPC Workflows
MIT License
118 stars 26 forks source link

freeze reqs #467

Closed bgunnar5 closed 6 months ago

bgunnar5 commented 6 months ago

Freezing the requirements for different versions of python. This will make it so that this version of Merlin can be installed with the same dependencies that we know should work. This should make it easier for users to revert to previous versions of Merlin without running into dependency issues.

EDIT: I'm purposefully putting this directly into main as develop should not have frozen requirements; that way we can keep up with the latest version of libraries on develop and ensure we have stable dependencies on main.

bgunnar5 commented 6 months ago

@lucpeterson @koning @doutriaux1 Are you all alright with the idea of freezing requirements for Merlin on the main branch? We can leave the requirements more open ended on develop so that we can stay up-to-date but I think this will make our releases more stable as our dependencies release updates.

koning commented 6 months ago

I think we will need to add the new celery update and that will dictate the versions.

bgunnar5 commented 6 months ago

Which new celery update are you referencing?

koning commented 6 months ago

This one: https://github.com/celery/celery/issues/8030

bgunnar5 commented 6 months ago

So maybe for now we should freeze the celery version to be 5.2.7 for all python versions rather than just python 3.7 like I currently have it set to.

bgunnar5 commented 6 months ago

Per @doutriaux1's request: @jwhite242 @LinaMuryanto do either of you have any thoughts regarding freezing requirements for a release? Could this potentially break the WEAVE venv?

LinaMuryanto commented 6 months ago

Per @doutriaux1's request: @jwhite242 @LinaMuryanto do either of you have any thoughts regarding freezing requirements for a release? Could this potentially break the WEAVE venv?

As of today (02/2024), WEAVE is pip installing merlin into the spack-based venv, so if the versions of dependencies are strict ("=="), and conflict with what were spack installed, merlin will not install into the spack-based venv. If you do ">=", it may be better. Instead of doing "pip install" of merlin into the spack-based venv, WEAVE can also spack install merlin, then spack will not look at the requirements.txt, it will look at the spec specified in py-merlin's package.py.

jwhite242 commented 6 months ago

Yeah, for those monolithic environments, frozen dependencies would definitely cause problems, as Lina noted. They are great for reproducible dev work, and with the github ci though where you're only exercising merlin. Maybe worth retaining non-frozen requirements with softer and more flexible bounds for the main build system and keep the second lock file for building environments with for the devs? Maestro does this with poetry where the lock file is separate, and only really gets activated if installing with poetry, but the pip installs get the more relaxed version.

One other comment that's bitten me recently with Maestro: this also hides incompatibilities due to dependency version changes since there's not really a great tool to do any combinatorial testing on versions outside the lock. Haven't found any tools that seem to support this kind of testing yet (though if anybody here knows of one, do share!).

bgunnar5 commented 6 months ago

I'm going to hold off on doing anything with the requirements for now. I'll come back to this at a future date but I think it's important to get the latest update out ASAP for users.