Add pytorch to the conda environment #8094
Conversation
|
Github actions tests reported job failures from actions build 3464982962
|
|
Yes! I am so relieved to have all of this python dependency wrangling done. Perhaps not the sexiest work, but still very important -- thank you @asmirnov239 ! |
|
Andrey is OOO, but maybe @lbergelson knows---is it expected that the CNN tests are failing (due to keras being missing)? I think I just need to get oriented about the order in which this and any other PRs (e.g., to replace or otherwise reorganize code for the CNN) are going to be merged. |
There was a problem hiding this comment.
Thanks, @asmirnov239! Some proposed changes and questions, but otherwise looks really good! Hope it wasn't too painful messing with the dependencies, and thanks for going through the exercise.
I trust that you have or will run additional scientific/pipeline-level tests---might be good to document or summarize results here, as well!
| @@ -0,0 +1,257 @@ | |||
| @HD VN:1.6 | |||
There was a problem hiding this comment.
This file was already added in #7889, correct?
| @@ -341,7 +341,7 @@ def disengage(self): | |||
| def engage(self): | |||
| try: | |||
| all_converged = False | |||
| while self.i_epoch <= self.hybrid_inference_params.max_training_epochs: | |||
| while self.i_epoch <= self.hybrid_inference_params.max_training_epochs: | |||
There was a problem hiding this comment.
Stray white space added?
| - pytorch=1.10.1 | ||
| - setuptools=59.5.0 | ||
|
|
||
| # other python dependencies; these should be removed after functionality is moved into Java code |
There was a problem hiding this comment.
If these lines weren't changed, perhaps restore their original location to minimize the diff?
| - conda-forge::numpy=1.17.5 # do not update, this will break scipy=1.0.0 | ||
| - conda-forge::mkl=2022.1.0 # MKL typically provides dramatic performance increases for theano, tensorflow, and other key dependencies | ||
| - conda-forge::mkl-service=2.4.0 | ||
| - conda-forge::numpy=1.21.2 # do not update, this will break scipy=1.0.0 |
There was a problem hiding this comment.
The comment about scipy is out of date.
| - conda-forge::scikit-learn=0.23.1 | ||
| - conda-forge::matplotlib=3.2.1 | ||
| - conda-forge::pandas=1.0.3 | ||
| - conda-forge::typing_extensions=4.1.1 # see https://github.com/broadinstitute/gatk/issues/7800 and linked PRs | ||
|
|
||
| - pytorch=1.10.1 |
There was a problem hiding this comment.
Any notes about this package that you might want to include in a comment? Which channel is it pulled from, and is there a reason not to pin that?
| @@ -70,6 +70,7 @@ ENV CLASSPATH /gatk/gatk.jar:$CLASSPATH | |||
| WORKDIR /gatk | |||
| RUN chmod -R a+rw /gatk | |||
| ENV PATH $CONDA_PATH/envs/gatk/bin:$CONDA_PATH/bin:$PATH | |||
| RUN conda update conda | |||
There was a problem hiding this comment.
Which version of conda do we end up with? Would it be more reproducible to just install that version directly in the base or pin it in some other way (is that possible here)?
| rest.__init_group__(unseen_free_RVs) | ||
| self.groups.append(rest) | ||
| self.model = model | ||
|
|
There was a problem hiding this comment.
Maybe even out the white space here and below the following class.
| op_kwargs={'temperature': temperature}) | ||
| KLThermal, approx, None, temperature=temperature) | ||
|
|
||
| class DeterministicApproximation(Approximation): |
There was a problem hiding this comment.
Some comments here about the determinism issues are definitely merited---summarize the problem in the original code and explain the fix here. Maybe also link the Slack thread---perhaps just on Github, rather than in code comments?
There was a problem hiding this comment.
| model = modelcontext(model) | ||
| if not model.free_RVs: | ||
| raise TypeError('Model does not have FreeRVs') | ||
| self.groups = list() |
There was a problem hiding this comment.
Just curious, is most of this code essentially borrowed from PyMC4 (as you referenced in the Slack thread above), or is it original?
|
@samuelklee @asmirnov239 Yes, the CNN tests failing here is completely expected! What we should probably do for now is deprecate the CNN tools, and move the legacy conda environment into a separate yml file that you have to activate manually if you need to run the old CNN tools for some reason. Then once NVidia implements a training tool to go along with |
This pull request updates the environment to include
pytorchto gatk conda environment. This required an update to numpy and consequently updates of PyMC3 and its dependencies, as well as parts of gCNV code.