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

Refactor to work with matchms 0.6.0 #35

Merged
merged 28 commits into from
Sep 16, 2020
Merged

Conversation

florian-huber
Copy link
Member

@florian-huber florian-huber commented Sep 10, 2020

Started simple and became somewhat bigger with the recent changes of matchms.

  • Include Python 3.8 (and CI for this).
  • Remove double CI runs (as with matchms before).
  • Integrate Spec2Vec and Spec2VecParallel and make them inherit from BaseSimilarity.
  • Remove now redundant parallel integration test.
  • Add tests for Spec2Vec.pair() and .matrix() methods.

@florian-huber florian-huber marked this pull request as ready for review September 10, 2020 18:14
@florian-huber florian-huber changed the title Refactor environment Refactor to work with matchms 0.6.0 Sep 14, 2020
@florian-huber
Copy link
Member Author

florian-huber commented Sep 14, 2020

Not yet sure why that is, but in the CI verify runs it seems to install the released spec2vec 0.2.0 from nlesc instead of the locally published package. And it does so only for Python 3.7.

Copy link
Member

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

The build and verify jobs do not have same matrix.

Also the upload artifact name does not include matrix identifiers making it possible to get wires crossed.

Let me do some local testing:

.github/workflows/conda_build.yml Outdated Show resolved Hide resolved
@sverhoeven
Copy link
Member

Luckliy a local conda build has the Spec2Vec class extending BaseSimilarity.

  • Tried conda install --channel bioconda --channel conda-forge --channel nlesc --channel $BUILDDIR -v --use-local spec2vec still installs spec2vec from nlesc channel.
  • Tried conda install --channel bioconda --channel conda-forge --channel nlesc --channel $BUILDDIR -v --only-deps spec2vec does not install matchms, so will not work.
  • Tried channel priorities with conda install --channel /tmp/ff/noarch --channel bioconda --channel conda-forge --channel nlesc --strict-channel-priority --override-channels -v spec2vec, but it got stuck resolving.

Seems https://anaconda.org/nlesc/spec2vec/0.2.0/download/noarch/spec2vec-0.2.0-pyhec00094_0.tar.bz2 and GH workflow /home/runner/work/_temp/spec2vec/_build/noarch/spec2vec-0.2.0-pyhec00094_0.tar.bz2 have the same version + build string so for conda it does not matter which is installed.
Changed conda/meta.yaml with perl -pi -e 's/number: 0/number: 999/' conda/meta.yaml

-number: 0
+number: 999

The install with conda install --channel bioconda --channel conda-forge --channel nlesc --channel $BUILDDIR -v spec2vec got install from local channel. spec2vec ff2/noarch::spec2vec-0.2.0-pyhec00094_999 instead of spec2vec nlesc/noarch::spec2vec-0.2.0-pyhec00094_0. I repeated command a couple of time (even with channel reorder) and it kept installing the 999 package.

So I think adding a step before https://github.com/iomega/spec2vec/blob/refactor_environment/.github/workflows/conda_build.yml#L125 to change the build number in conda/meta.yml should fix it.

@florian-huber
Copy link
Member Author

Thanks a lot @sverhoeven . That looks better than the hard coded fix I had added in the end (installing matchms first, and then spec2vec).

@florian-huber
Copy link
Member Author

It also worked for me, locally. But here it still fails and picks spec2vec from nlesc.

@sverhoeven
Copy link
Member

sverhoeven commented Sep 15, 2020

Grr, can you add listing for /home/runner/work/_temp/spec2vec/_build and ${RUNNER_TEMP}/spec2vec/_build/noarch in build and verify job respectivly? I want to check that the 999 package is available in verify job.

@florian-huber
Copy link
Member Author

florian-huber commented Sep 16, 2020

Seems to me like the file in /noarch is present as expected: spec2vec-0.2.0-pyhec00094_999.tar.bz2.
The "solution" of first installing matchms and then spec2vec did work, so as a backup we could move back to that. (it feels a bit weird though to hard code this into the conda recipe, but OK...)

conda install --channel bioconda --channel conda-forge --channel nlesc matchms
conda install --channel bioconda --channel conda-forge --channel $BUILDDIR spec2vec

[edit]:
We could also add matchms to the local build folder so that we would mimic a local nlesc channel and could only run conda install --channel bioconda --channel conda-forge --channel $BUILDDIR spec2vec ?

@sverhoeven
Copy link
Member

Agree, let's rollback the build string 999 change and go with conda install matchms + conda install spec2vec then we don't have to depend on the conda resolving algorithm.

Copy link
Member

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

Great to see the CI green again.

Just have some minor improvements.

.github/workflows/conda_build.yml Show resolved Hide resolved
.github/workflows/conda_build.yml Outdated Show resolved Hide resolved
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@florian-huber florian-huber merged commit f2a7253 into master Sep 16, 2020
@florian-huber florian-huber deleted the refactor_environment branch September 16, 2020 17:28
@florian-huber
Copy link
Member Author

Thanks a lot for the help, Stefan!

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.

2 participants