openkinome / kinoml

Structure-informed machine learning for kinase modeling
https://openkinome.org/kinoml/
MIT License
52 stars 21 forks source link

[WIP] Attempt to simplify test environment YAML #58

Closed jchodera closed 2 years ago

jchodera commented 3 years ago

Description

This PR attempts to streamline the conda test environment YAML.

Todos

Questions

Status

codecov-commenter commented 3 years ago

Codecov Report

Merging #58 (876f9bb) into master (b46bd19) will decrease coverage by 0.02%. The diff coverage is n/a.

t-kimber commented 2 years ago

I tried using this conda installation for pytorch geometric (which requires Pytorch >= 1.8.0 installed) :

https://pytorch-geometric.readthedocs.io/en/latest/notes/installation.html

jchodera commented 2 years ago

Oh no, the OpenEye segmentation fault is back:

kinoml/tests/modeling/test_oemodeling.py::test_read_electron_density[kinoml.data.electron_densities-4f8o_phases.mtz-expectation0-396011] 
/home/runner/work/_temp/dbb87906-42c1-426f-a2b0-4e6029cf2825.sh: line 1:  2326 Segmentation fault      (core dumped) pytest -v --cov=kinoml --cov-report=xml --color=yes kinoml/tests/

This is presumably due to some kind of library incompatibility in one of the installed dependencies. It could be that the openeye libraries are just built against the wrong non-conda-forge-compatible libraries, which would be a pain.

jchodera commented 2 years ago

Maybe we could try adding a couple more versions of Python (supported by conda-forge) to see if this is systematic across all python versions?

schallerdavid commented 2 years ago

I tested the segmentation fault different python versions (3.7, 3.8, 3.9) and operating systems (Linux, MacOS). It only shows up in the linux installations.

schallerdavid commented 2 years ago

Interestingly, I do not see the segmentation fault when running the tests locally on a linux wsl2 under windows 11.

schallerdavid commented 2 years ago

However, the more concerning problem is that the second test (test_3rdparty_imports) fails. You can see it in the logs of the MacOS runs or locally when running the tests on linux. It fails when importing torch_geometric:

>>> import torch_geometric
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/david/miniconda3/envs/test/lib/python3.9/site-packages/torch_geometric/__init__.py", line 7, in <module>
    import torch_geometric.data
  File "/home/david/miniconda3/envs/test/lib/python3.9/site-packages/torch_geometric/data/__init__.py", line 1, in <module>
    from .data import Data
  File "/home/david/miniconda3/envs/test/lib/python3.9/site-packages/torch_geometric/data/data.py", line 3, in <module>
    from torch_geometric.typing import OptTensor, NodeType, EdgeType
  File "/home/david/miniconda3/envs/test/lib/python3.9/site-packages/torch_geometric/typing.py", line 4, in <module>
    from torch_sparse import SparseTensor
  File "/home/david/miniconda3/envs/test/lib/python3.9/site-packages/torch_sparse/__init__.py", line 19, in <module>
    torch.ops.load_library(spec.origin)
  File "/home/david/miniconda3/envs/test/lib/python3.9/site-packages/torch/_ops.py", line 110, in load_library
    ctypes.CDLL(path)
  File "/home/david/miniconda3/envs/test/lib/python3.9/ctypes/__init__.py", line 382, in __init__
    self._handle = _dlopen(self._name, mode)
OSError: /home/david/miniconda3/envs/test/lib/python3.9/site-packages/torch_sparse/_version_cuda.so: undefined symbol: _ZN5torch3jit17parseSchemaOrNameERKSs

Several issues were already raised about this (e.g. #673). The fixing solutions are usually about checking if multiple pytorch versions are installed or about trying different gcc versions. But this is impractical on github actions I guess.

schallerdavid commented 2 years ago

Installing via pytorch_geometric instead of pyg does not help either. Returning to pyg for now, since it appears to be the recommended way of installing pytorch_geometric (documentation).

schallerdavid commented 2 years ago

@jchodera I figured out a proper solution to install pytorch_geometric via mamba. It is all about channels and their ordering. To get a proper installation we need channels in the following order:

However, this will also lead to the segmentation fault when reading in electron densities (only on linux). Since we need electron densities for e.g. iridium score, there is not a proper solution yet. We could try again, once the new openeye toolkit is released or message the openeye folks about it. Also, I am currently not using the electron densities in the structural modeling featurizers. So we could out comment the electron density functionality until it is properly working and I use a separate environment to get the iridium scores for the docking benchmark.

jchodera commented 2 years ago

This is great!

We can certainly comment out the electron density fitting tests (or perhaps mark them to exclude them from github actions tests?) for now.

I'm surprised we need the pytorch channel, since that is a distinct build from the conda-forge channel build. Perhaps the pyg channel build of pytorch-geometric is building from the pytorch channel instead of conda-forge?

schallerdavid commented 2 years ago

Allright, I'll try again without the pytorch channel, but on my local machine it seemed to be required.

schallerdavid commented 2 years ago

Okay, so pytorch_geometric can be installed via the pyg channel, we also need the pytorch channel. The test for read_electron_density is disabled for now due to a segmentation fault, likely arising from incompatible libraries between the openeye-toolkit and other dependencies.

ToDo

Status

schallerdavid commented 2 years ago

@jchodera I cant assign you as a reviewer since you created this PR. Are you fine with merging it the way it is now or do you have any other suggestions?

t-kimber commented 2 years ago

Hi @schallerdavid , thanks for looking into this! Just a quick question: we were initially only testing the CI on Linux and MacOS for Python 3.7 and 3.9. I see that it's also being tested for 3.8. Is there a particular reason for adding it?

schallerdavid commented 2 years ago

@t-kimber Thanks for your feedback! The main reason to include more Python versions across different operating systems was to test the segfault we got from the OpenEye toolkit. I would keep them for now, since the test are run in parallel, so fewer python versions will not decrease the speed of the tests.

t-kimber commented 2 years ago

I notice that the master branch hasn't been pulled into this branch. We might need to revert the merging, @schallerdavid .

schallerdavid commented 2 years ago

@t-kimber I merged master in this branch yesterday evening, also all tests are passing. Where do you see the conflicts?

t-kimber commented 2 years ago

No conflicts, all good!