Skip to content

{bio}[intel/2017a] dammit 0.3.2 /w Python 2.7.13 (REVIEW)#5125

Merged
boegel merged 15 commits intoeasybuilders:developfrom
hajgato:dammit032
Jan 6, 2018
Merged

{bio}[intel/2017a] dammit 0.3.2 /w Python 2.7.13 (REVIEW)#5125
boegel merged 15 commits intoeasybuilders:developfrom
hajgato:dammit032

Conversation

@hajgato
Copy link
Copy Markdown
Collaborator

@hajgato hajgato commented Sep 15, 2017

Needs #5119 #5120 #5121 #5122 #5123 #5124
I would also like to discuss the naming conventions in the PR and the dep PRs, I want clearly avoid suffix -Ruby-2.3.4-Perl-5.24.1-Python-2.7.13 ...

homepage = 'http://matplotlib.org'
description = """matplotlib is a python 2D plotting library which produces publication quality figures in a variety of
hardcopy formats and interactive environments across platforms. matplotlib can be used in python scripts, the python
and ipython shell, web application servers, and six graphical user interface toolkits."""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@hajgato homepage + descr needs fixing?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel Done

('crb-blast', '0.6.9', '-Ruby-2.3.4'),
]


Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

one empty line is sufficient

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel Done

is that LAST finds initial matches based on their multiplicity,
instead of using a fixed length (e.g. BLAST uses 11-mers)."""

toolchain = {'name': 'GCCcore', 'version': '6.3.0'}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hmm, wouldn't intel make more sense here, for performance reasons?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel Good question, I am afraid of intel compiler in biostuff.


name = 'dammit'
version = '0.3.2'
versionsuffix = '-Python-%(pyver)s'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Either include Python, Perl & Ruby in the versionsuffix, or none of them...

Since the versionsuffix would become very long here, I consider it OK to just drop it here as an exception to our rule of including Python/Perl version in there...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel I included Python, because dammit is actually written in Python. It have deps needs Ruby and Perl. Anyway, I will drop all of them.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, OK, in that case I guess it makes sense to keep it.

Python is definitely way more common than Perl or Ruby, so it kind of makes sense to make only that one explicit in the versionsuffix...

@easybuilders easybuilders deleted a comment from boegelbot Oct 12, 2017
@easybuilders easybuilders deleted a comment from boegelbot Oct 12, 2017
@easybuilders easybuilders deleted a comment from boegelbot Oct 12, 2017
@easybuilders easybuilders deleted a comment from boegelbot Oct 12, 2017
@hajgato hajgato changed the title {bio}[intel/2017a] dammit 0.3.2 /w Python 2.7.13 (REVIEW) {bio}[intel/2017a] dammit 0.3.2 /w Python 2.7.13 (WIP) Oct 18, 2017
@hajgato
Copy link
Copy Markdown
Collaborator Author

hajgato commented Oct 18, 2017

Apparently BUSCO needs Python3

@hajgato hajgato changed the title {bio}[intel/2017a] dammit 0.3.2 /w Python 2.7.13 (WIP) {bio}[intel/2017a] dammit 0.3.2 /w Python 2.7.13 (REVIEW) Oct 27, 2017
@boegel boegel added this to the 3.5.1 milestone Dec 30, 2017
'checksums': ['6ab8ff5c19e7f452966bf5a3220b845cf3244fe0b96544f7f9acedcc2db5c705'],
}),
(name, version, {
'patches': ['dammit-0.3.2_nodocs.patch', 'dammit-0.3.2_py2busco.diff'],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@hajgato Please rename dammit-0.3.2_py2busco.diff to dammit-0.3.2_py2busco.patch, and include one patch per line:

 'patches': [
    'dammit-0.3.2_nodocs.patch',
    'dammit-0.3.2_py2busco.patch',
],

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel: done

instead of using a fixed length (e.g. BLAST uses 11-mers)."""

toolchain = {'name': 'intel', 'version': '2017a'}
toolchainopts = {'cstd': 'c++11', 'opt': True}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@hajgato Why the 'opt': True (which enables using -O3 rather than -O2), any particular reason for this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel: taken form last-869/makefile, line 1:

CXXFLAGS = -O3 -std=c++11 -pthread -DHAS_CXX_THREADS

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK so you're trying to mimic this behaviour, I see.

In general, -O3 is less "safe" than -O2 though (in terms of precision, etc.).

And the benefit is usually fairly small.

sources = ['%(namelower)s-%(version)s.zip']

patches = ['%(name)s-%(version)s_makefile.patch']

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please drop this empty line, and the one above as well

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel: done

'b7b697b048e22c913eb196449eb895fc70b22fb72db5cca1f8dc948cde164309', # LAST-869_makefile.patch
]

builddependencies = [('binutils', '2.27')]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is not needed when using a full toolchain like intel, it's already included

'doc',
'examples',
'README.txt',
'src',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why also copy the sources?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel: some executables in the src dir, dammit directly calls some of those (for example src/last-split and others, I do not remember specifically.)

@@ -0,0 +1,52 @@
easyblock = 'MakeCp'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have an existing easyconfig for LAST (for a more recent version) which uses ConfigureMake and does make install prefix=%(installdir)s.

But maybe that's not supported yet with this older version of LAST?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel: this version has only a simple makefile, no configure provided.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

source_urls = ['http://last.cbrc.jp/']
sources = ['%(namelower)s-%(version)s.zip']

patches = ['%(name)s-%(version)s_makefile.patch']
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There should be no need for a patch, in the existing easyconfig for LAST we use:

buildopts = 'CC="$CC" CXX="$CXX" CFLAGS="$CFLAGS" CXXFLAGS="$CXXFLAGS -pthread -DHAS_CXX_THREADS"'

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel: CC, CXX, CFLAGS, and CXXFLAGS hard encoded in last-869/src/makefile, lines 1-9:

CXX = g++
CC  = gcc

CXXFLAGS = -O3 -Wall -Wextra -Wcast-qual -Wswitch-enum -Wundef	\
-Wcast-align -pedantic -g -std=c++11 -pthread -DHAS_CXX_THREADS
# -Wconversion
# -fomit-frame-pointer ?

CFLAGS = -Wall -O2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Even with a makefile like this, you can override values by passing arguments to make, like we do in the existing LAST easyconfig...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel Done


source_urls = ['https://github.com/TransDecoder/TransDecoder/archive/']
sources = ['v%(version)s.tar.gz']

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

style nitpicking: please drop empty line

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel: Done

]

sanity_check_paths = {
'files': ['TransDecoder.%s' % x for x in ['Predict', 'LongOrfs']],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need to use a list comprehension here:

    'files': ['TransDecoder.LongOrfs', 'TransDecoder.Predict'],

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel Done

@easybuilders easybuilders deleted a comment from boegelbot Dec 30, 2017
@easybuilders easybuilders deleted a comment from boegelbot Dec 30, 2017
@easybuilders easybuilders deleted a comment from boegelbot Dec 30, 2017
@easybuilders easybuilders deleted a comment from boegelbot Dec 30, 2017
@boegel
Copy link
Copy Markdown
Member

boegel commented Jan 5, 2018

Test report by @boegel
SUCCESS
Build succeeded for 3 out of 3 (3 easyconfigs in this PR)
node2092.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/d126a099889574ae756d26fc8e9f2e46 for a full test report.

@boegel
Copy link
Copy Markdown
Member

boegel commented Jan 6, 2018

Test report by @boegel
SUCCESS
Build succeeded for 3 out of 3 (3 easyconfigs in this PR)
node2401.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/1c1f3a48cbf49a641dd238e5e3190e3b for a full test report.

@boegel
Copy link
Copy Markdown
Member

boegel commented Jan 6, 2018

Going in, thanks @hajgato!

@boegel boegel merged commit 09a0ac5 into easybuilders:develop Jan 6, 2018
@hajgato hajgato deleted the dammit032 branch February 12, 2018 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants