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

Add pysirius client to conda-forge #22016

Merged
merged 25 commits into from
Nov 13, 2023
Merged

Conversation

mfleisch
Copy link
Contributor

@mfleisch mfleisch commented Feb 13, 2023

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/pysirius) and found some lint.

Here's what I've got...

For recipes/pysirius:

  • noarch: python recipes are required to have a lower bound on the python version. Typically this means putting python >=3.6 in both host and run but you should check upstream for the package's Python compatibility.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/pysirius) and found it was in an excellent condition.

recipes/pysirius/meta.yaml Outdated Show resolved Hide resolved
recipes/pysirius/meta.yaml Outdated Show resolved Hide resolved
@mfleisch mfleisch marked this pull request as draft March 30, 2023 12:46
@LukasScholz
Copy link
Contributor

I hereby confirm that I am willing to be listed as a Maintainer

works locally. Unsure if os.getenv('RECIPE_DIR') points to the correct folder
@mfleisch mfleisch marked this pull request as ready for review April 17, 2023 12:01
@mfleisch
Copy link
Contributor Author

@conda-forge/staged-recipes ready for review

@mfleisch
Copy link
Contributor Author

@conda-forge/help-python ready for review

@mfleisch
Copy link
Contributor Author

@synapticarbors can you please have a look if the pr is ready to be merged.

@jaimergp jaimergp closed this Oct 21, 2023
@jaimergp jaimergp reopened this Oct 21, 2023
@jaimergp
Copy link
Member

Taking a look now, but CI needs to rerun, since it's been a while. In the meantime, let me know if those suggestions are useful. Just a typo and trying to catch some errors early, but nothing critical.

Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
@joXemMx
Copy link
Contributor

joXemMx commented Oct 22, 2023

@jaimergp Thanks a lot for taking a look! I have included your suggestions and got the CI to run successfully again.

Copy link
Member

@jaimergp jaimergp left a comment

Choose a reason for hiding this comment

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

The errors were about the u in set -euxo pipefail (unbound variables result in error). You can still do set -ex if you want to at least have failures error out early, but up to you (it's just usually easier to debug when things go wrong).

That aside, the logs show that you are packaging your tests as test/*.py, which would result in a top-level package name test (e.g. import test), which is going to cause trouble with other packages due to clobbering and other conflicts:

  adding 'test/__init__.py'
  adding 'test/test_account_credentials.py'
  adding 'test/test_account_info.py'
  adding 'test/test_annotated_peak.py'
  adding 'test/test_annotated_spectrum.py'
  adding 'test/test_canopus.py'
  adding 'test/test_canopus_predictions.py'
  ...

Your options are:

  • Do not package your tests as a top-level package, but as a subpackage PySirius.tests or similar
  • Exclude test in pyproject.toml or setup.py, and package them as part of the conda tests via test.files in meta.yaml.
  • Do not package them at all (delete before running pip install or exclude from pyproject.toml or setup.py).

Comment on lines 39 to 40
- sh run_test.sh # [not win]
- call run_test.bat # [win]
Copy link
Member

Choose a reason for hiding this comment

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

Having these scripts named run_test.* tells conda-build (counterintuitively) to ignore the test.commands section. Please rename to something else like test_script.* or something like that so `pip check does run.

@joXemMx
Copy link
Contributor

joXemMx commented Oct 22, 2023

@jaimergp Thanks again! I decided to not package the test files you mentioned at all and also adjusted to your other suggestions. Is there something else you notice that needs changing?

@joXemMx
Copy link
Contributor

joXemMx commented Oct 23, 2023

@jaimergp Do you reckon the package is ready for merge? I realize the feedstock will not make use of the Windows test per default, but still I would like to keep it in as this allows me to temporarily enable the test during pull requests by setting the corresponding noarch_platforms in conda-forge.yml.

@jaimergp jaimergp closed this Nov 13, 2023
@jaimergp jaimergp reopened this Nov 13, 2023
- python-dateutil >=2.5.3
- setuptools >=21.0.0
- urllib3 >=1.15.1
- sirius-ms
Copy link
Member

Choose a reason for hiding this comment

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

Just double checking: you don't need to be specific about sirius-ms versions here, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really at this stage, but I added it for completeness. We probably will want to raise limits for the sirius-ms version in later releases.

@jaimergp jaimergp dismissed synapticarbors’s stale review November 13, 2023 10:38

Requested changes were addressed.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/pysirius) and found some lint.

Here's what I've got...

For recipes/pysirius:

  • requirements: run: sirius-ms >= 5.7.0 should not contain a space between relational operator and the version, i.e. sirius-ms >=5.7.0

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/pysirius) and found it was in an excellent condition.

@joXemMx
Copy link
Contributor

joXemMx commented Nov 13, 2023

@jaimergp ready to merge from my side

@@ -0,0 +1,57 @@
{% set version = "0.9" %}
Copy link
Member

Choose a reason for hiding this comment

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

If this is version 0.9, why is pip reporting:

Successfully installed PySirius-1.0.0

🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@jaimergp This is caused due to swagger generating the code as a "package" with version 1.0.0. It does not matter that it does that, as the conda-forge package is still successfully build as py-sirius-ms 0.9 (see i.e. line 437 in linux_64 "Run docker build"). When generating a new client API in the future, swagger will still call it PySirius-1.0.0, but our package may be py-sirius-ms 1.3 at that point.
TL;DR: we can ignore swaggers versioning and use our own

Copy link
Contributor

Choose a reason for hiding this comment

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

but anyways, I will adjust it shortly so it matches

Copy link
Member

Choose a reason for hiding this comment

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

Yea, this is not a blocker for the merge. You can amend it in-feedstock in the future.

@jaimergp jaimergp merged commit a7bf5f9 into conda-forge:main Nov 13, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants