Make sure that AutoGroups work correctly also when concurrent processes are executed #3650

giovannipizzi · 2019-12-12T16:50:56Z

If many verdi run processes were run at the same time, there were cases in which the automatic group name generated by the AutoGroup was conflicting, resulting in IntegrityErrors from the database.
This issue fixes this problem by automatically generating a new name until this is unique.
Moreover, many bugs and issues found while addressing this issue are solved.

Fixes #997

giovannipizzi · 2019-12-12T16:52:09Z

I would suggest not to squash the two commits - I am not 100% sure if the first, alone, would pass all tests, but the changes are splitted in two logical units.

If this is an issue, ok to squash, but then please keep both commit messages as they contain relevant information.

In particular: - deprecated use of 'group-name' in favour of 'group-label' both in the internal API of AutoGroups and in the CLI parameters of `verdi run` - Improved the Autogroup implementation and API - add various tests of the functionality of autogroups - fixed the CLI parameters to include/exclude groups, that were badly broken (and untested). Now it accepts any entrypoint string (before it was accepting the first part of an node_type string). Also added click validation of the parameters. - fixed the --group flag to activate/deactivate autogrouping (it was wrongly defined and thus always True) - linter fixes

giovannipizzi · 2020-04-01T16:21:38Z

I've rebased this, so it's ready to review (hoping the tests didn't break in the meantime...). @ramirezfranciscof do you want to go through this (together, or alone)?

Also, @sphuber, looking back I realised that I improved the performance of storing nodes.
After we merge, could you please re-run the test to see how many simulations you can submit at max rate, so we get a feeling if this change already gave an important performance boost? (it might!)

Also, let me reiterate the comment on not squashing, if possible.

codecov · 2020-04-02T08:05:30Z

Codecov Report

Merging #3650 into develop will increase coverage by 0.82%.
The diff coverage is 84.69%.

@@             Coverage Diff             @@
##           develop    #3650      +/-   ##
===========================================
+ Coverage    77.18%   78.00%   +0.82%     
===========================================
  Files          457      457              
  Lines        33795    33830      +35     
===========================================
+ Hits         26084    26390     +306     
+ Misses        7711     7440     -271

Flag	Coverage Δ
#django	`70.05% <84.69%> (+0.83%)`	⬆️
#sqlalchemy	`70.87% <84.69%> (+0.82%)`	⬆️

Impacted Files	Coverage Δ
aiida/tools/ipython/ipython_magics.py	`0.00% <0.00%> (ø)`
aiida/cmdline/commands/cmd_plugin.py	`25.00% <66.66%> (ø)`
aiida/backends/testbase.py	`93.39% <75.00%> (-1.51%)`	⬇️
aiida/plugins/entry_point.py	`88.18% <80.00%> (+0.10%)`	⬆️
aiida/orm/autogroup.py	`86.23% <85.00%> (+16.12%)`	⬆️
aiida/cmdline/commands/cmd_run.py	`89.55% <86.36%> (-3.44%)`	⬇️
aiida/engine/processes/calcjobs/calcjob.py	`82.26% <100.00%> (+3.39%)`	⬆️
aiida/manage/caching.py	`99.28% <100.00%> (ø)`
aiida/orm/nodes/node.py	`91.21% <100.00%> (+0.34%)`	⬆️
aiida/orm/utils/node.py	`92.07% <100.00%> (ø)`
... and 15 more

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 161eeae...9a68100. Read the comment docs.

giovannipizzi · 2020-04-02T09:40:17Z

Sorry, it's taking a bit longer than expected as I found a new unexpected problem in the tests for backwards-migrations.
I have identified now the problem (a backward migration wasn't resetting correctly the migration after being run) and I have just force-pushed a commit that should fix this problem and actually also improve the code style of that specific class.
I have also fixed a few more prospector complaints along the way (for some reason unrelated to my changes, but it was complaining locally...).

greschd · 2020-04-02T09:55:05Z

Maybe related to the pre-commit and migration issues encountered in #3738?

giovannipizzi · 2020-04-02T10:12:54Z

Maybe related to the pre-commit and migration issues encountered in #3738?

Most probably - let me fix it here (actually it's now fixed, I'm just fixing another hopefully final pylint complaint, that BTW I see was also addressed in #3738.

I've already force-pushed, if it works I'd suggest to merge this first.

BTW, I hate when you fix all problems and then prospector starts complaining of something new... ;-)

giovannipizzi · 2020-04-02T10:49:54Z

Tests pass!

@sphuber @ramirezfranciscof this is ready for review, and @ltalirz this should also fix the migration test problem (or at least I fixed a problem in the same place you saw problems in #3738)

sphuber · 2020-04-02T10:57:08Z

Before I start reviewing, @giovannipizzi I am afraid you may have made a mistake in your rebasing. You now have two commits, but both with the same commit message

giovannipizzi · 2020-04-02T11:05:57Z

Before I start reviewing, @giovannipizzi I am afraid you may have made a mistake in your rebasing. You now have two commits, but both with the same commit message

Oh no... how was the reflog trick to get back old commit messages? Anybody knows?

giovannipizzi · 2020-04-02T11:11:25Z

Never mind, I think I found it. I'll force-push with the correct message.

This also remove an overzelous isinstance check, and moves additional checks in a cached function that is run only when storing the very first node (that needs to be put in an autogroup), making storing of nodes faster (even if times oscillates so it's hard to estimate exactly by how much). Also, added logic to allow for concurrent creation of multiple groups (and test). This fixes aiidateam#997

giovannipizzi · 2020-04-02T11:16:32Z

Ok @sphuber I fixed the commit message.
I hope I didn't mix up the content of the two commits - in the worst case, ok to squash but keeping text from both commit messages.

sphuber

Thanks @giovannipizzi . Have some comments, maybe easiest to discuss in person on skype/zoom?

aiida/cmdline/commands/cmd_run.py

aiida/backends/testbase.py

aiida/cmdline/commands/cmd_run.py

aiida/orm/autogroup.py

giovannipizzi · 2020-04-03T11:55:16Z

Addressed all comments in a new commit. Let's see how many rounds of testing I'll need to do this time...

sphuber · 2020-04-03T12:04:23Z

You should be able to run the docs and pre-commit also locally. For docs make -C docs html and for pre-commit pre-commit run --all. Then you won't have to push multiple times and depend on the CI

giovannipizzi · 2020-04-03T12:46:47Z

Yes, thanks.
I know these two - for the docs, I was too optimistic and didn't think to have broken it.

For the pre-commit, the problem is that if I check only the problematic files, I get no error... (with pre-commit run --files tests/cmdline/commands/test_run.py).

I could run --all-files but it takes (on my PC) ~2 min spinning all CPUs at 100%.
Most problematic is the fact that if I run on all files I get errors that I don't see if I just check a single file, and even more weirdly I get errors in new parts of the code randomly popping out (e.g., I just run and I got this:

aiida/tools/ipython/ipython_magics.py
  Line: 37
    pylint: no-name-in-module / No name 'version_info' in module 'IPython'
  Line: 38
    pylint: import-error / Unable to import 'IPython.core'
    pylint: no-name-in-module / No name 'core' in module 'IPython'

I'll fix this (and I already fixed something similar earlier) but it's just very confusing.

BTW, a third test (sqlalchemy, py3.5) failed because it couldn't upload to codecov... Did anybody already see it? Mentioning @csadorf for info

In particular, the most important changes include: - now the auto-group flag is called `--auto-group`, is a flag and is False by default - only kept `--include` and `--exclude` options, checking typestrings and allowing to end with `%`. One benefit is that while reimplementing I replaced some `isinstance` with string comparisons, with potential further benefits. - `--include` and `--exclude` are now mutually exclusive - improved documentation of the main point of the issue

sphuber · 2020-04-03T12:59:25Z

BTW, a third test (sqlalchemy, py3.5) failed because it couldn't upload to codecov... Did anybody already see it? Mentioning @csadorf for info

This I have also already seen multiple times. For me was also py3.5 but probably just coincidence. Seems like a transient connection problem. Would be nice if this could be couched in a retry or if it could not fail the test. But then again, the coverage probably depends on it.

sphuber

Thanks Gio. Unfortunately I still have some questions. Happy to discuss in person

aiida/cmdline/commands/cmd_run.py

aiida/orm/autogroup.py

Most notable: now the % sign can be anywhere in the string of included or excluded classes, and not only at the end.

giovannipizzi · 2020-04-03T15:29:10Z

All comments addressed and tests pass.

giovannipizzi · 2020-04-03T21:52:12Z

This PR is cursed ;-) Fixed the problems after the merge, now ready to be merged!

sphuber · 2020-04-04T17:57:29Z

Let the curse be gone! Thanks @giovannipizzi !

giovannipizzi requested a review from sphuber December 12, 2019 16:50

ramirezfranciscof self-assigned this Apr 1, 2020

giovannipizzi force-pushed the fix_997_parallel_autogroups branch from 8bea787 to aeded55 Compare April 1, 2020 16:19

giovannipizzi requested a review from ramirezfranciscof April 1, 2020 16:19

giovannipizzi force-pushed the fix_997_parallel_autogroups branch from aeded55 to 825521f Compare April 2, 2020 07:53

giovannipizzi force-pushed the fix_997_parallel_autogroups branch 2 times, most recently from 0264ff7 to 5d9120a Compare April 2, 2020 09:38

giovannipizzi force-pushed the fix_997_parallel_autogroups branch from 5d9120a to 972efcd Compare April 2, 2020 10:10

giovannipizzi mentioned this pull request Apr 2, 2020

Clean up top level directory of repository #3738

Merged

giovannipizzi mentioned this pull request Apr 2, 2020

seemingly random changes in pre-commit #3885

Closed

giovannipizzi force-pushed the fix_997_parallel_autogroups branch from 972efcd to 43d852e Compare April 2, 2020 11:13

sphuber requested changes Apr 2, 2020

View reviewed changes

giovannipizzi force-pushed the fix_997_parallel_autogroups branch from 3ac609e to ba4b217 Compare April 3, 2020 12:55

sphuber requested changes Apr 3, 2020

View reviewed changes

A few further comments addressed.

31d0470

Most notable: now the % sign can be anywhere in the string of included or excluded classes, and not only at the end.

sphuber and others added 2 commits April 3, 2020 19:27

Merge branch 'develop' into fix_997_parallel_autogroups

a63cc52

Minor pre-commit fixes after merge.

9a68100

sphuber approved these changes Apr 4, 2020

View reviewed changes

sphuber merged commit f40f0d2 into aiidateam:develop Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure that AutoGroups work correctly also when concurrent processes are executed #3650

Make sure that AutoGroups work correctly also when concurrent processes are executed #3650

giovannipizzi commented Dec 12, 2019

giovannipizzi commented Dec 12, 2019

giovannipizzi commented Apr 1, 2020

codecov bot commented Apr 2, 2020 •

edited

Loading

giovannipizzi commented Apr 2, 2020

greschd commented Apr 2, 2020

giovannipizzi commented Apr 2, 2020

giovannipizzi commented Apr 2, 2020

sphuber commented Apr 2, 2020

giovannipizzi commented Apr 2, 2020

giovannipizzi commented Apr 2, 2020

giovannipizzi commented Apr 2, 2020

sphuber left a comment

giovannipizzi commented Apr 3, 2020

sphuber commented Apr 3, 2020

giovannipizzi commented Apr 3, 2020

sphuber commented Apr 3, 2020

sphuber left a comment

giovannipizzi commented Apr 3, 2020

giovannipizzi commented Apr 3, 2020

sphuber commented Apr 4, 2020

Make sure that AutoGroups work correctly also when concurrent processes are executed #3650

Make sure that AutoGroups work correctly also when concurrent processes are executed #3650

Conversation

giovannipizzi commented Dec 12, 2019

giovannipizzi commented Dec 12, 2019

giovannipizzi commented Apr 1, 2020

codecov bot commented Apr 2, 2020 • edited Loading

Codecov Report

giovannipizzi commented Apr 2, 2020

greschd commented Apr 2, 2020

giovannipizzi commented Apr 2, 2020

giovannipizzi commented Apr 2, 2020

sphuber commented Apr 2, 2020

giovannipizzi commented Apr 2, 2020

giovannipizzi commented Apr 2, 2020

giovannipizzi commented Apr 2, 2020

sphuber left a comment

Choose a reason for hiding this comment

giovannipizzi commented Apr 3, 2020

sphuber commented Apr 3, 2020

giovannipizzi commented Apr 3, 2020

sphuber commented Apr 3, 2020

sphuber left a comment

Choose a reason for hiding this comment

giovannipizzi commented Apr 3, 2020

giovannipizzi commented Apr 3, 2020

sphuber commented Apr 4, 2020

codecov bot commented Apr 2, 2020 •

edited

Loading