Skip to content

{bio}[foss/2018b] seq2HLA v20180802#6969

Merged
boegel merged 3 commits intoeasybuilders:developfrom
smoors:20181005165854_new_pr_seq2HLA20180802
Oct 12, 2018
Merged

{bio}[foss/2018b] seq2HLA v20180802#6969
boegel merged 3 commits intoeasybuilders:developfrom
smoors:20181005165854_new_pr_seq2HLA20180802

Conversation

@smoors
Copy link
Copy Markdown
Contributor

@smoors smoors commented Oct 5, 2018

(created using eb --new-pr)

Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

@smoors It looks like we were working on this in parallel, see #6965...

There's a couple of differences, biggest one being the version; also I have the R dep missing and have seq2HLA.py --help as a sanity check command.

easyblock = 'Tarball'

name = 'seq2HLA'
version = '20180802'
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.

@smoors I think it makes sense to go with 2.3 here, since that's what this (fairly old) commit clearly points to.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@boegel fine by me, I'm just a little worried that the code for this version might change because it is not a real release.

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.

@smoors Note that in #6965 I'm also installing by commit ID, so it's "safe".

]

postinstallcmds = [
'sed -i -e "1i #!/usr/bin/env python" %(installdir)s/seq2HLA.py',
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.

@smoors You should also enable execution permissions (to avoid that users have to use python $EBROOTSEQ2HLA/seq2HLA.py and can just run seq2HLA.py)?

Copy link
Copy Markdown
Contributor Author

@smoors smoors Oct 7, 2018

Choose a reason for hiding this comment

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

it already has execute permissions, so this is not necessary.
I guess your plan is to go further with my PR? I will add your sanity check command then.
we can also keep the both PRs, since yours is with a different toolchain.

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.

#6965 already got merged, but it indeed makes sense to go forward with both because of the different toolchains.

Do you mind syncing this PR with develop, and adding the missing R dependency in the seq2HLA easyconfig that got merged in #6965?

I guess you can drop the post-install command I added for the permissions too since they're fine already.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure, but shouldn't you create a Python2 version of R with intel/2018a first?

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.

That's a good point, ugh...

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.

@smoors After thinking about this a bit, I'm not sure it's worth that effort...

Let's go forward in this PR with including R as dependency for seq2HLA, but not for older toolchains. As far as I understand, R is very optional anyway as dependency here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think R is optional actually. but that should not hold this PR back, right? :)

@boegel boegel added new update and removed new labels Oct 5, 2018
@boegel
Copy link
Copy Markdown
Member

boegel commented Oct 11, 2018

@smoors seq2HLA-20180802-foss-2018b-Python-2.7.15.eb should be removed?

@boegel boegel added this to the 3.7.1 milestone Oct 12, 2018
@boegel
Copy link
Copy Markdown
Member

boegel commented Oct 12, 2018

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

@boegel
Copy link
Copy Markdown
Member

boegel commented Oct 12, 2018

Test report by @boegel
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in this PR)
node3126.skitty.os - Linux centos linux 7.5.1804, Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz, Python 2.7.5
See https://gist.github.com/8267064f8d2973a5a0199011eb4942e6 for a full test report.

@boegel
Copy link
Copy Markdown
Member

boegel commented Oct 12, 2018

Test report by @boegel
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in this PR)
node2614.swalot.os - Linux centos linux 7.5.1804, Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz, Python 2.7.5
See https://gist.github.com/fbfbf7495cfaba2c7489948f9b181e14 for a full test report.

@boegel
Copy link
Copy Markdown
Member

boegel commented Oct 12, 2018

Going in, thanks @smoors!

@boegel boegel merged commit 68af30f into easybuilders:develop Oct 12, 2018
@smoors smoors deleted the 20181005165854_new_pr_seq2HLA20180802 branch January 27, 2020 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants