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

{bio}[foss/2020b} pangolin v3.1.10 #13733

Merged

Conversation

sassy-crick
Copy link
Collaborator

@sassy-crick sassy-crick commented Aug 13, 2021

(created using eb --new-pr)

edit: requires #13708 (UShER), #13651 (gofasta)

@sassy-crick sassy-crick changed the title pangolin 3.1.10 created {BIO}[foss/2020b} pangolin 3.1.10 created Aug 13, 2021
@sassy-crick sassy-crick changed the title {BIO}[foss/2020b} pangolin 3.1.10 created {bio}[foss/2020b} pangolin 3.1.10 created Aug 13, 2021
Micket
Micket previously requested changes Aug 13, 2021
}),
('constellations', '0.0.13', {
'source_urls': ['https://github.com/%(github_account)s/%name)s/archive'],
'sources': [{'download_filename': 'v%(version).tar.gz', 'filename': '%(name)s-%(version)s.tar.gz'}],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'sources': [{'download_filename': 'v%(version).tar.gz', 'filename': '%(name)s-%(version)s.tar.gz'}],
'sources': [{'download_filename': 'v%(version)s.tar.gz', 'filename': '%(name)s-%(version)s.tar.gz'}],

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for finding them! I simply did not spot them. (I do need new glasses, not a joke). I have corrected them.
By the way, is there a way to avoid these repeating lines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Converted to draft as we are waiting to have UShER merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, is there a way to avoid these repeating lines?

You can likely put them into exts_default_options (dict that is merged with this dict) although I'm not fully sure it works with the templates, but please try.
Also use SOURCE_TAR_GZ as the filename and GITHUB_SOURCE as the source_url, see https://docs.easybuild.io/en/latest/version-specific/easyconfig_templates.html

Converted to draft as we are waiting to have UShER merged.

If that is the only thing then simply mention it in the PR description like: - [ ] Requires #13708

@sassy-crick sassy-crick marked this pull request as draft August 13, 2021 19:56
@boegel boegel added the new label Aug 14, 2021
@boegel boegel added this to the 4.x milestone Aug 14, 2021
@Flamefire
Copy link
Contributor

The dependencies and extensions seem a bit off. I guess you used https://github.com/cov-lineages/pangolin/blob/master/environment.yml as a reference?
Checking https://github.com/cov-lineages/pangolin/blob/504d9833f4715c26b08b30e1cd2ad859d358c721/setup.py#L14-L22 there seem to be a few Python packages missing. E.g. pandas. Can you double check those?

@SethosII
Copy link
Contributor

@Flamefire pandas for example is included in SciPy-bundle

@sassy-crick
Copy link
Collaborator Author

The dependencies and extensions seem a bit off. I guess you used https://github.com/cov-lineages/pangolin/blob/master/environment.yml as a reference?
Yes, that is what I done and also added what was missing when I installed it. This is where the scikit-learn for example came from.
@SethosII has already addressed the issue with pandas I will look into the other two I noticed joblib and PuLP and see if they are really missing or being pulled in.
I noticed that for scikit-learn SciPy-bundle is a requirement so we could remove that one.

@Flamefire
Copy link
Contributor

I noticed that for scikit-learn SciPy-bundle is a requirement so we could remove that one.

Keep it. It is better to have the direct dependencies explicit in case the others change in a later version

@sassy-crick
Copy link
Collaborator Author

sassy-crick commented Aug 22, 2021

The dependencies and extensions seem a bit off. I guess you used https://github.com/cov-lineages/pangolin/blob/master/environment.yml as a reference?
Yes, that is what I done and also added what was missing when I installed it. This is where the scikit-learn for example came from.

@SethosII has already addressed the issue with pandas. I have checked this link and compared it with what I got:

pandas                        1.1.4  (>=1.0.1)
wheel                         0.35.1 (>=0.34)
joblib                        0.17.0 (>=0.11)
pysam                         0.16.0.1 (>1.16.0)
scikit-learn                  0.23.2 (>= 0.23.1)
PuLP                          2.4 ((>=2)

So that should be ok.

I looked into getting the repeating lines in the extension list a bit more neat. I replaced the URLs with the GITHUB_SOURCE as suggested. However, the SOURCE_TAR_GZ will only work in one case as all the others are named v-name-version.tar.gz. I tried to put the checksums outside the exts_list but here the style-check failed.

@sassy-crick sassy-crick changed the title {bio}[foss/2020b} pangolin 3.1.10 created {bio}[foss/2020b} pangolin 3.1.10 created Requires #13708 or #13694 Aug 22, 2021
@sassy-crick
Copy link
Collaborator Author

Additionally, to move forward, added Kent_tools so I can removed them from UShER

@sassy-crick sassy-crick changed the title {bio}[foss/2020b} pangolin 3.1.10 created Requires #13708 or #13694 {bio}[foss/2020b} pangolin 3.1.10 created Aug 27, 2021
@sassy-crick sassy-crick marked this pull request as ready for review August 27, 2021 18:56
@easybuilders easybuilders deleted a comment from boegelbot Aug 28, 2021
@boegel boegel changed the title {bio}[foss/2020b} pangolin 3.1.10 created {bio}[foss/2020b} pangolin v3.1.10 Aug 28, 2021
@boegel boegel modified the milestones: 4.x, next release (4.4.2?) Aug 28, 2021
@boegel
Copy link
Member

boegel commented Aug 28, 2021

Test report by @boegel
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in total)
node3582.doduo.os - Linux RHEL 8.2, x86_64, AMD EPYC 7552 48-Core Processor (zen2), Python 3.6.8
See https://gist.github.com/7821359089b03a595ecfa5f248427bae for a full test report.

@boegel
Copy link
Member

boegel commented Aug 28, 2021

@boegelbot please test @ generoso

@boegelbot
Copy link
Collaborator

@boegel: Request for testing this PR well received on generoso

PR test command 'EB_PR=13733 EB_ARGS= /apps/slurm/default/bin/sbatch --job-name test_PR_13733 --ntasks=4 ~/boegelbot/eb_from_pr_upload_generoso.sh' executed!

  • exit code: 0
  • output:
Submitted batch job 18231

Test results coming soon (I hope)...

- notification for comment with ID 907594993 processed

Message to humans: this is just bookkeeping information for me,
it is of no use to you (unless you think I have a bug, which I don't).

@boegelbot
Copy link
Collaborator

Test report by @boegelbot
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in total)
generoso-c1-s-6 - Linux centos linux 8.2.2004, x86_64, Intel(R) Xeon(R) CPU E5-2667 v3 @ 3.20GHz (haswell), Python 3.6.8
See https://gist.github.com/0f1dda9fae82bc6f524dfc2608182eb0 for a full test report.

@boegel
Copy link
Member

boegel commented Aug 28, 2021

Test report by @boegel
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in total)
node2609.swalot.os - Linux centos linux 7.9.2009, x86_64, Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz (haswell), Python 3.6.8
See https://gist.github.com/77c558db9afc772baa7b54fce897ade2 for a full test report.

@boegel boegel dismissed Micket’s stale review August 28, 2021 09:26

typos fixed

@@ -0,0 +1,80 @@
# Contribution from the NIHR Biomedical Research Centre
# Guy's and St Thomas' NHS Foundation Trust and King's College London
# Based on a EC provided by Paul Jähn
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nitpick: my last name has an e at the end ;)

Suggested change
# Based on a EC provided by Paul Jähn
# Based on a EC provided by Paul Jähne

'dirs': ['lib/python%(pyshortver)s/site-packages'],
}

sanity_check_commands = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also add pangolin -v:

Suggested change
sanity_check_commands = [
sanity_check_commands = [
'pangolin -v',

Copy link
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.

lgtm

@boegel
Copy link
Member

boegel commented Aug 28, 2021

Going in, thanks @sassy-crick!

@boegel boegel merged commit 14e889b into easybuilders:develop Aug 28, 2021
@sassy-crick sassy-crick deleted the 20210813113839_new_pr_pangolin3110 branch August 28, 2021 15:00
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.

6 participants