{bio}[foss/2016b] QUAST v4.6.0 by SethosII · Pull Request #5610 · easybuilders/easybuild-easyconfigs

SethosII · 2018-01-09T12:09:07Z

(created using eb --new-pr)

I used installopts to give setup.py the option install_full. This results in setup.py install install_full .... It uses install_full but is there a way to overwrite install?

One other thing: QUAST has several optional dependencies, I only included matplotlib for now.

Yet another thing: normally, modules using Python and Perl have the appropriate versionsuffix. Should QUAST have both as a suffix?

edit: requires

~~Add Time::HiRes to recent Perl versions #5616~~
~~support install_target in PythonPackage + deprecate use_easy_install & use_setup_py_develop easybuild-easyblocks#1341~~

boegel · 2018-01-09T18:51:33Z

@SethosII

the setup.py install part is currently hardcoded in PythonPackage (see https://github.com/easybuilders/easybuild-easyblocks/blob/master/easybuild/easyblocks/generic/pythonpackage.py#L56), but it would make sense to add a custom easyconfig parameter install_target (with install as default value when setup.py is used) so you can tweak it... Are you up for looking into that?
We usually try to include as many optional dependencies as possible, but that highly depends on the amount of effort required to provide them, so it's largely up to you. Any optional dependencies you can include are a win, but don't spend tons of time on it unless you really have a need for them, I'd say.
Since QUAST is primarily a Python package, I think just including Python in the versionsuffix is sufficient.

SethosII · 2018-01-10T10:56:49Z

@boegel

I will modify the PythonPackage EasyBlock. Allowing a custom install_target would make use_setup_py_develop obsolete (and maybe also the use_easy_install parameter)?
One optional dependency is the Perl moduleTime::HiRes, should I add it as a separate EasyConfig or include it in the Perl EasyConfig?

boegel · 2018-01-10T13:36:49Z

The custominstall_target should allow you to specify something else than install when SETUP_PY_INSTALL_CMD is used.

With that supported, you can consider dropping the SETUP_PY_DEVELOP_CMD constant and implementing it via SETUP_PY_INSTALL_CMD + setting install_target to develop, and likewise for EASY_INSTALL_INSTALL_CMD.

You should not drop support for use_setup_py_develop or use_easy_install though, since existing easyconfigs may be relying on that.

Changing the implementation has to be done with a bit of care too, since other easyblocks (e.g. ones not included in the central repository) may be relying on the SETUP_PY_DEVELOP_CMD and EASY_INSTALL_INSTALL_CMD constants, so removing those may results in problems too...
It's OK to add Time::HiRes in the Perl easyconfig imho, as long as you do it consistently (i.e. in all recent Perl easyconfigs). That's pretty easy to test via eb --skip, so not a huge issue to submit test reports. Just do it in a separate pull request.

SethosII · 2018-01-11T09:41:45Z

@boegel I added the optional dependencies and created a pull request for the Perl module and the install_target option.

boegel · 2018-01-11T09:57:48Z

+
+# bypass import check, sanity check fails without this
+# 'ImportError: No module named quast' during sanity check
+options = {'modulename': 'site'}


Are there no Python installed? There seem to be, since you're checking for a non-empty lib/python%(pyshortver)s/site-packages/?

If there are, please use a 'proper' module name here rather than site which is always there.

The site-packages directory only contains this:

$ ls /easybuild/test/software/QUAST/4.6.0-foss-2016b-Python-3.5.2/lib/python3.5/site-packag es/ easy-install.pth joblib-0.11-py3.5.egg __pycache__ quast-4.6.0-py3.5.egg simplejson-3.13.2-py3.5-linux-x86_64.egg site.py

So I don't think that we can provide a real module to check for.

I think you need to check for import quast_libs, so try this (and drop the comment above):

options = {'modulename': 'quast_libs'}

Thanks, that worked.

boegel · 2018-01-11T09:58:37Z

@SethosII For future updates, please don't use git push --force, it can be interesting to have changes made in the git history for future reference.

SethosII · 2018-01-11T10:30:35Z

@boegel I will do so.

boegel · 2018-01-11T11:29:18Z

@SethosII How is the installation different if you use setup.py install vs setup.py install_full?

I just tested this without the updated PythonPackage, which results in ignoring install_target, but the sanity check passes just fine...

Can you enhance the sanity check to check for the full installation?

SethosII · 2018-01-11T13:57:31Z

@boegel Additional libraries are built with install_full. These are silva and manta. I added them and the others to the sanity_check_paths.

Add correct modulename Add check for libraries

boegel

lgtm

boegel · 2018-01-12T20:58:13Z

Test report by @boegel
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in this PR)
node2400.golett.os - Linux centos linux 7.4.1708, Intel(R) Xeon(R) CPU E5-2680 v3 @ 2.50GHz, Python 2.7.5
See https://gist.github.com/4880b6ee354bb5a5f6b856706baa1a07 for a full test report.

boegel · 2018-01-12T20:59:24Z

Test report by @boegel
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in this PR)
node2004.delcatty.os - Linux centos linux 7.4.1708, Intel(R) Xeon(R) CPU E5-2670 0 @ 2.60GHz, Python 2.7.5
See https://gist.github.com/b649fc2903986644daa121ad4ac74da7 for a full test report.

boegel · 2018-01-12T21:11:37Z

Tested on top of easybuilders/easybuild-easyblocks#1341, looking good.

boegel · 2018-01-12T21:11:51Z

Going in, thanks @SethosII!

boegel · 2018-04-24T11:19:34Z

@SethosII I'm looking into problems with BioPerl and proovread, which I think are caused by inclusing Time::HiRes`` into the list of extensions for Perl` in this PR...

As far as I can tell Time::HiRes as a Perl core module, so there should be no need to include it as an extension.

In addition, the version you included (01.02) is quite strange (I didn't notice that when reviewing this PR)...
Time::HiRes versions have been 1.97xy for a while now, see https://metacpan.org/source/JHI/Time-HiRes-1.9758/Changes .

How did you get to 01.02 as version, and what was the trigger to include Time::HiRes as an extension, there should be no need since it's a core module?

SethosII · 2018-04-25T05:30:56Z

@boegel As far as I can tell based on my browser history I included it because QUAST has Time::HiRes listed as an optional dependency. I wasn't aware of it being a perl core module which it is since 5.7.3:

$ corelist -a Time::HiRes

Data for 2015-12-13
Time::HiRes was first released with perl v5.7.3
...

So I searched for it and this link was the first CPAN link I found. This CPAN page has a link to the latest version, but I seem to have ignored this too. So there is no need for this, sorry for including this and causing trouble down the line.

boegel · 2018-04-25T06:51:44Z

@SethosII Thanks for clarifying, and no worries about it! We should have noticed this during the regression test of EasyBuild v3.5.1, but I clearly wasn't thorough enough back then...

We're rolling back adding Time::HiRes as an extension in #6225, which fixes the problem with BioPerl and proovread. I made sure this doesn't affect the installation of QUAST.

boegel added this to the 3.5.1 milestone Jan 9, 2018

boegel added the new label Jan 9, 2018

boegel modified the milestones: 3.5.1, 3.6.0 Jan 11, 2018

adding easyconfigs: QUAST-4.6.0-foss-2016b-Python-3.5.2.eb

f5e13b3

SethosII force-pushed the 20180109130859_new_pr_QUAST460 branch from 2942ec3 to f5e13b3 Compare January 11, 2018 09:40

This was referenced Jan 11, 2018

Add Time::HiRes to recent Perl versions #5616

Merged

support install_target in PythonPackage + deprecate use_easy_install & use_setup_py_develop easybuilders/easybuild-easyblocks#1341

Merged

boegel reviewed Jan 11, 2018

View reviewed changes

Improve sanity check

e172639

Add correct modulename Add check for libraries

SethosII force-pushed the 20180109130859_new_pr_QUAST460 branch from c79b1e0 to e172639 Compare January 11, 2018 13:58

boegel approved these changes Jan 11, 2018

View reviewed changes

boegel modified the milestones: 3.6.0, 3.5.1 Jan 12, 2018

boegel merged commit 843d376 into easybuilders:develop Jan 12, 2018

SethosII deleted the 20180109130859_new_pr_QUAST460 branch January 15, 2018 06:03

boegel mentioned this pull request Apr 24, 2018

don't include (ancient version of) Time::HiRes as Perl extension, since it's a core Perl module #6225

Merged

Conversation

SethosII commented Jan 9, 2018 • edited by boegel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boegel commented Jan 9, 2018

Uh oh!

SethosII commented Jan 10, 2018

Uh oh!

boegel commented Jan 10, 2018

Uh oh!

SethosII commented Jan 11, 2018

Uh oh!

boegel Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

SethosII Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

boegel Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

SethosII Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

boegel commented Jan 11, 2018

Uh oh!

SethosII commented Jan 11, 2018

Uh oh!

boegel commented Jan 11, 2018

Uh oh!

SethosII commented Jan 11, 2018

Uh oh!

boegel left a comment

Choose a reason for hiding this comment

Uh oh!

boegel commented Jan 12, 2018

Uh oh!

boegel commented Jan 12, 2018

Uh oh!

boegel commented Jan 12, 2018

Uh oh!

boegel commented Jan 12, 2018

Uh oh!

boegel commented Apr 24, 2018

Uh oh!

SethosII commented Apr 25, 2018

Uh oh!

boegel commented Apr 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SethosII commented Jan 9, 2018 •

edited by boegel

Loading