👌 IMPROVE: Add account option to LsfScheduler

[sphuber]

No description provided.

[codecov]

Codecov Report

Merging #4832 (dc21993) into develop (13f95ca) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #4832      +/-   ##
===========================================
+ Coverage    79.54%   79.58%   +0.04%     
===========================================
  Files          519      519              
  Lines        37092    37093       +1     
===========================================
+ Hits         29502    29517      +15     
+ Misses        7590     7576      -14     

Flag	Coverage Δ
django	74.32% <100.00%> (+0.04%)	⬆️
sqlalchemy	73.23% <100.00%> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/schedulers/plugins/lsf.py	70.08% <100.00%> (+5.66%)	⬆️
aiida/transports/plugins/local.py	81.54% <0.00%> (-0.25%)	⬇️
aiida/schedulers/scheduler.py	76.25% <0.00%> (+0.63%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13f95ca...dc21993. Read the comment docs.

[ramirezfranciscof]

Mmm I know this is a bit of a PITA because most of the these would be more related to the error messages you are fixing than the actual addition of account, but could you add the tests for the sections that codecov is complaining about?

BTW: not sure if you requested from @chrisjsewell because you wanted him to check or test for anything specific, the PR looks rather straightforward to me, so I could approve it once the tests are there if you want.

[sphuber]

Fine to be reviewed by anyone that feels confident. It isn't a difficult change but some experience with the LSF scheduler would be useful. Don't really have time to be adding tests for these exception cases though.

[ramirezfranciscof]

Ah, ok, I see. In that case yes, we can wait for @chrisjsewell review. In the meantime, if you don't mind, I could try adding up the tests for these exception raises.

[sphuber]

if you don't mind, I could try adding up the tests for these exception raises.

That'd be great. Please feel free to go ahead

[ramirezfranciscof]

@sphuber did you allow for modifications in your PR-branch? I can't seem to push changes to it. I'm trying the following:

(aiida-full) workuser@apolos-Air aiida-core % git status

On branch pr4832_lsf
nothing to commit, working tree clean

(aiida-full) workuser@apolos-Air aiida-core % git remote -vv

fork	https://ramirezfranciscof@github.com/ramirezfranciscof/aiida-core.git (fetch)
fork	https://ramirezfranciscof@github.com/ramirezfranciscof/aiida-core.git (push)
sphuber	https://ramirezfranciscof@github.com/sphuber/aiida-core (fetch)
sphuber	https://ramirezfranciscof@github.com/sphuber/aiida-core (push)

(aiida-full) workuser@apolos-Air aiida-core % git push --set-upstream sphuber:fix/lsf-scheduler-add-account-option

ssh: Could not resolve hostname sphuber: nodename nor servname provided, or not known
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

[sphuber]

You are even already added as a collaborator on my fork, but I think it wouldn't even be necessary in principle.
How did you add my fork as a remote? The output of your git remote call seems weird. What if you try to set it to:

git remote set-url sphuber https://github.com/sphuber/aiida-core

and then push your branch as

git push pr4832_lsf sphuber:fix/lsf-scheduler-add-account-option

[ramirezfranciscof]

The output of your git remote call seems weird.

What do you mean "weird"? Do you say for the ... ramirezfranciscof@... part? I set that so that I don't need to enter my username every time I push to a remote, it just asks for my pass.

I anyways tried changing it and push how you suggested and didn't work. This seemed to do the trick though:

git push sphuber HEAD:fix/lsf-scheduler-add-account-option

I was suspecting it would be related to the million different ways in which git tries to deduce which are the source branch and target repo/branch, of which apparently I'm not yet super clear on how they work. Anyhow, it worked now, so as you were...

(EDIT: I got the solution from here, just for reference)

[ramirezfranciscof]

Sorry, I thought that since I was adding tests and the testsuite was not very complex I would migrate the format to pytest, but apparently this complicated a bit github's understanding of the changes.

If it helps, I think you can just ignore test_parse_common_joblist_output, or just verify that the only thing I did was taking it out of the class (idem for test_submit_output and test_kill_output).

[sphuber]

Thanks a lot @ramirezfranciscof . For me this would be good to merge

[chrisjsewell]

Yep all good in the hood 👍

[chrisjsewell]

grr @aiidateam/dependency-manager, pip has suddenly decided that it can't resolve dependencies for pip install .[all]: https://github.com/aiidateam/aiida-core/pull/4832/checks?check_run_id=2286189589

[csadorf]

grr @aiidateam/dependency-manager, pip has suddenly decided that it can't resolve dependencies for pip install .[all]: https://github.com/aiidateam/aiida-core/pull/4832/checks?check_run_id=2286189589

Looks like it might just be timing out? 😕

[chrisjsewell]

Looks like it might just be timing out? 😕

Yeh thats because it can't work out compatible dependencies, it just goes on forever downloading exponential numbers of package versions. If you can workout what package has changed since it was working, and pin that, then this behaviour will disappear

[csadorf]

Looks like it might just be timing out? 😕

Yeh thats because it can't work out compatible dependencies, it just goes on forever downloading exponential numbers of package versions. If you can workout what package has changed since it was working, and pin that, then this behaviour will disappear

I just ran an analysis and found that the following packages had releases since yesterday:

• django
• sqlalchemy
• tqdm
• pre-commit

Of those only tqdm and pre-commit could be affected since django and sqlalchemy are constrained to older versions.

For the record, the following packages had releases in the past 7 days:

• django
• sqlalchemy
• tqdm
• docutils
• pre-commit
• pg8000
• pytest

[chrisjsewell]

thanks @csadorf, at a guess I reckon it could be this: tlocke/pg8000@16b08f3

[chrisjsewell]

nope no joy

[csadorf]

nope no joy

I tried to constrain tqdm, pre-commit, pg8000, and pytest, but no luck so far.

[csadorf]

I've ran pipenv against the requirements with all extras and found the following conflict:

[pipenv.exceptions.ResolutionFailure]: Warning: Your dependencies could not be resolved. You likely have a mismatch in your sub-dependencies.
  First try clearing your dependency cache with $ pipenv lock --clear, then try the original command again.
 Alternatively, you can use $ pipenv install --skip-lock to bypass this mechanism, then run $ pipenv graph to inspect the situation.
  Hint: try $ pipenv lock --pre if it is a pre-release dependency.
ERROR: Could not find a version that matches coverage<5.0,>=5.2.1 (from -r /tmp/pipenv4885zi1irequirements/pipenv-tqjn69z8-constraints.txt (line 47))
Tried: 3.0, 3.0.1, 3.1, 3.2, 3.3, 3.3.1, 3.4, 3.5, 3.5.1, 3.5.2, 3.5.3, 3.6, 3.7, 3.7.1, 4.0, 4.0, 4.0.1, 4.0.1, 4.0.2, 4.0.2, 4.0.3, 4.0.3, 4.1, 4.1, 4.2, 4.2, 4.3, 4.3.1, 4.3.2, 4.3.3, 4.3.4, 4.4, 4.4.1, 4.4.2, 4.5, 4.5.1, 4.5.2, 4.5.3, 4.5.4, 5.0, 5.0.1, 5.0.2, 5.0.3, 5.0.4, 5.1, 5.2, 5.2, 5.2.1, 5.2.1, 5.3, 5.3, 5.3.1, 5.3.1, 5.3.1, 5.4, 5.4, 5.4, 5.5, 5.5, 5.5
Skipped pre-versions: 3.0b3, 3.1b1, 3.2b1, 3.2b2, 3.2b3, 3.2b4, 3.4b1, 3.4b2, 3.5b1, 3.5.1b1, 3.5.2b1, 3.6b1, 3.6b2, 3.6b3, 4.0a1, 4.0a2, 4.0a3, 4.0a4, 4.0a5, 4.0a5, 4.0a5, 4.0a6, 4.0a6, 4.0a6, 4.0b1, 4.0b1, 4.0b2, 4.0b2, 4.0b3, 4.0b3, 4.1b1, 4.1b1, 4.1b2, 4.1b2, 4.1b3, 4.1b3, 4.2b1, 4.2b1, 4.4b1, 5.0a1, 5.0a2, 5.0a3, 5.0a4, 5.0a5, 5.0a6, 5.0a7, 5.0a8, 5.0b1, 5.0b2
There are incompatible versions in the resolved dependencies:
  coverage<5.0 (from -r /tmp/pipenv4885zi1irequirements/pipenv-tqjn69z8-constraints.txt (line 47))
  coverage>=5.2.1 (from pytest-cov==2.11.1->-r /tmp/pipenv4885zi1irequirements/pipenv-tqjn69z8-constraints.txt (line 72))

[chrisjsewell]

I've ran pipenv against the requirements with all extras and found the following conflict

nice cheers, out of interest how long did this take to identify the conflict?

[csadorf]

I've ran pipenv against the requirements with all extras and found the following conflict

nice cheers, out of interest how long did this take to identify the conflict?

Just a few minutes. Let's continue discussion relating to this particular problem in the associated issue: #4850