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

sci-biology/multiqc: add ebuild #1223

Closed
wants to merge 6 commits into from
Closed

sci-biology/multiqc: add ebuild #1223

wants to merge 6 commits into from

Conversation

mschubert
Copy link
Contributor

Add the MultiQC tool which is often used for aggregating sequencing quality control reports.

This includes a few dependencies that are currently not in the science overlay:

  • dev-python/colormath
  • dev-python/spectra
  • dev-python/lzstring
  • dev-python/rich-click
  • dev-python/pyaml-env

Currently, everything is in one commit but I'll separate them out once any additional issues are resolved.

@mschubert mschubert marked this pull request as ready for review December 19, 2023 09:38
@mschubert
Copy link
Contributor Author

This is ready for review

(except that commits are not split yet)

@Nowa-Ammerlaan
Copy link
Member

Thanks for the PR, at first glance this looks fine.

However, I'm a bit hesitant to add new things to this repository, because in general this repository is not well maintained. I merge the occasional PR, but the time frame in which this happens strongly depends on my availability and ::sci is usually at the bottom of my priority list (as I believe it is for many others). Would you be open to pushing this to our user repository ::guru instead? I think it will be easier for you to keep this functional and up-to-date if it is maintained there, and it will also be less work for me. As an added bonus ::guru is covered by our automated tinderbox CI, while ::sci is not. Feel free to take whatever dependencies you need from ::sci and move them to ::guru if required, please let me know which ones you take so I can drop them here to prevent duplication.

(same holds for you other two PRs, code looks fine, but I would prefer to not add packages here if possible)

@mschubert
Copy link
Contributor Author

Thanks for your answer and please excuse the late response (I was away for the Christmas break). I see your points to redirect the PRs to ::guru. However, how do you think about the following:

Currently, if I'm a scientist wanting to use scientific software in Gentoo that's not in the main tree, I look at the ::sci repository. Even if I didn't know, searching google for something like "gentoo scientific software" points me to ::sci. It would not be obvious to me at all to search for ::guru for a subset of the same software, and I assume the same is true for many users. (All PRs are exclusively about scientific software and their dependencies)

The only semantic reason to move this kind of software to ::guru would be if ::sci was being deprecated and the idea was to slowly let ::guru take over all ebuilds that are still useful from ::sci. Is this the case? And if yes, where is this documented?

Of course, there is also the pragmatic reason that ::guru is simply better maintained (etc.) than ::sci, and PRs could be reviewed by more maintainers there. However, I'd be a bit hesitant to erode the documented reason for ::sci's existence (and user discoverability) based on the pragmatic reasoning alone.

@Nowa-Ammerlaan
Copy link
Member

It's not really properly documented anywhere, but we do have this bug: https://bugs.gentoo.org/693364

It's in a better state then it was when this bug was filed, but still many ebuilds are broken and I don't have the time to fix them all. Nor do I have time to check what works, and move these elsewhere. Instead I just merge whatever PRs come in, and ensure the CI is green.

By the way, I find that the best way to find some software that is not in ::gentoo is https://gpo.zugaina.org/. Users can find your ebuilds here, regardless of which overlay it is in.

@APN-Pucky
Copy link
Contributor

APN-Pucky commented Jan 3, 2024

ensure the CI is green

Given ::sci is not part of the standard gentoo ci, I could setup a github workflow that also compiles the package (just for new PRs), obviously that requires many prebuild packages, but given gentoo has an official binhost now that should even be fast within docker.

@Nowa-Ammerlaan
Copy link
Member

ensure the CI is green

Given ::sci is not part of the standard gentoo ci, I could setup a github workflow that also compiles the package (just for new PRs), obviously that requires many prebuild packages, but given gentoo has an official binhost now that should even be fast within docker.

We can ask ago to add ::sci to tinderbox. He offered to do this before, but I think this only makes sense if someone is actually going to fix the bugs that it will find.

@APN-Pucky
Copy link
Contributor

We can ask ago to add ::sci to tinderbox. He offered to do this before, but I think this only makes sense if someone is actually going to fix the bugs that it will find.

Maybe we can just add some categories first and step by step extend it? Then we won't flood them with failures.

@mschubert
Copy link
Contributor Author

only makes sense if someone is actually going to fix the bugs

I'd be happy to invest some time for this if you're going to keep ::sci running; I agree with Joel's comment here.

dev-python/rich[${PYTHON_USEDEP}]
dev-python/typing-extensions[${PYTHON_USEDEP}]"

distutils_enable_tests pytest
Copy link
Member

Choose a reason for hiding this comment

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

We are either missing some test dependency here, or we may need to explicitly set some PYTHONPATH:

____________________________________ ERROR collecting tests/test_help.py _____________________________________
ImportError while importing test module '/tmp/portage/dev-python/rich-click-1.7.2/work/rich-click-1.7.2/tests/test_help.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.11/site-packages/_pytest/python.py:617: in _importtestmodule
mod = import_path(self.path, mode=importmode, root=self.config.rootpath)
/usr/lib/python3.11/site-packages/_pytest/pathlib.py:567: in import_path
importlib.import_module(module_name)
/usr/lib/python3.11/importlib/__init__.py:126: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1204: in _gcd_import
???
<frozen importlib._bootstrap>:1176: in _find_and_load
???
<frozen importlib._bootstrap>:1147: in _find_and_load_unlocked
???
<frozen importlib._bootstrap>:690: in _load_unlocked
???
/usr/lib/python3.11/site-packages/_pytest/assertion/rewrite.py:186: in exec_module
exec(co, module.__dict__)
tests/test_help.py:9: in <module>
from tests.conftest import AssertRichFormat, AssertStr, InvokeCli
E   ModuleNotFoundError: No module named 'tests.conftest'


RDEPEND="dev-python/pyyaml[${PYTHON_USEDEP}]"

distutils_enable_tests pytest
Copy link
Member

Choose a reason for hiding this comment

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

Either there are no tests, or they are not available in the pypi tarball. Either remove this line if there are no test, or switch the SRC_URI to the github tarball:

============================================ test session starts =============================================
platform linux -- Python 3.11.7, pytest-7.4.4, pluggy-1.3.0 -- /tmp/portage/dev-python/pyaml-env-1.2.1/work/pyaml_env-1.2.1-python3_11/install/usr/bin/python3.11
cachedir: .pytest_cache
rootdir: /tmp/portage/dev-python/pyaml-env-1.2.1/work/pyaml_env-1.2.1
plugins: pkgcore-0.12.24, anyio-4.2.0
collecting ... collected 0 items

=========================================== no tests ran in 0.01s ============================================
* ERROR: dev-python/pyaml-env-1.2.1::localrepo failed (test phase):


RDEPEND="dev-python/colormath[${PYTHON_USEDEP}]"

distutils_enable_tests pytest
Copy link
Member

Choose a reason for hiding this comment

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

============================================================ ERRORS =============================================================
______________________________________________ ERROR collecting test/test_basic.py ______________________________________________
ImportError while importing test module '/tmp/portage/dev-python/spectra-0.0.11/work/spectra-0.0.11/test/test_basic.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.11/site-packages/_pytest/python.py:617: in _importtestmodule
mod = import_path(self.path, mode=importmode, root=self.config.rootpath)
/usr/lib/python3.11/site-packages/_pytest/pathlib.py:567: in import_path
importlib.import_module(module_name)
/usr/lib/python3.11/importlib/__init__.py:126: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1204: in _gcd_import
???
<frozen importlib._bootstrap>:1176: in _find_and_load
???
<frozen importlib._bootstrap>:1147: in _find_and_load_unlocked
???
<frozen importlib._bootstrap>:690: in _load_unlocked
???
/usr/lib/python3.11/site-packages/_pytest/assertion/rewrite.py:186: in exec_module
exec(co, module.__dict__)
test/test_basic.py:2: in <module>
import nose
E   ModuleNotFoundError: No module named 'nose'

Requires nose, please add RESTRICT="test"

dev-python/humanize[$PYTHON_USEDEP]
dev-python/pyaml-env[$PYTHON_USEDEP]"

distutils_enable_tests pytest
Copy link
Member

Choose a reason for hiding this comment

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

Same problem here:

====================================================== test session starts ======================================================
platform linux -- Python 3.11.7, pytest-7.4.4, pluggy-1.3.0 -- /tmp/portage/sci-biology/multiqc-1.19/work/multiqc-1.19-python3_11/install/usr/bin/python3.11
cachedir: .pytest_cache
rootdir: /tmp/portage/sci-biology/multiqc-1.19/work/multiqc-1.19
plugins: pkgcore-0.12.24, anyio-4.2.0
collecting ... collected 0 items

===================================================== no tests ran in 0.27s =====================================================
* ERROR: sci-biology/multiqc-1.19::localrepo failed (test phase):

Copy link
Member

@Nowa-Ammerlaan Nowa-Ammerlaan left a comment

Choose a reason for hiding this comment

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

Alright, I found some time to test this and left some comments.

In general:

  • Please add python3_12 compat to the packages. We are going to switch to 3.12 this year and almost everything in this repository still has to be tested and ported. So it will save lots of work later if we add 3.12 now. For bonus points you could also test and add pypy3 compat.
  • Did you also test these packages on x86? If not please remove those keywords and leave only the ones the packages are tested on.
  • Please add a Signed-off-by: line to your commit messages in accordance with our copyright policy.
  • Please test all packages with FEATURES=test and USE=test, especially when adding 3.12 and/or pypy3 to the compat list. The former enables the test phase, but the later is a USE flag that ensures the package is built with support for the tests and/or ensures the test dependencies are pulled in. The identical name is a bit confusing, but you need to enable both to execute the tests.

(These kind of issues would have been found automatically in ::guru, this is why I prefer adding new packages there)

@Nowa-Ammerlaan
Copy link
Member

Ooh and it would be great if you could split the commit into one commit per package. You can use the dev-util/pkgdev tool for this, it saves a lot of work because it automatically formats the commit message correctly and signs it off.

Signed-off-by: Michael Schubert <[email protected]>
Signed-off-by: Michael Schubert <[email protected]>
Signed-off-by: Michael Schubert <[email protected]>
Signed-off-by: Michael Schubert <[email protected]>
Signed-off-by: Michael Schubert <[email protected]>
Signed-off-by: Michael Schubert <[email protected]>
@mschubert
Copy link
Contributor Author

I've addressed all the comments as far as I could:

  • I've mostly disabled the test because they either depended on nose, or were not included in the PyPI tarball (and switching to the Github tarball always led to other issues)
  • Python 12 compat is added (and tested) where possible; lzstring does not support it, and so multiqc can't either
  • Removed the x86 flags
  • Added signoffs
  • Split the commits

This should be good now! Sorry about missing the FEATURES=test USE=test.

@Nowa-Ammerlaan
Copy link
Member

Thanks 👍

@mschubert mschubert deleted the multiqc branch June 12, 2024 11:23
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.

3 participants