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.

Sign up for GitHub

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

Jump to bottom

Replace unnecessary use of aiida_profile_clean #5814

Merged
merged 2 commits into from
Dec 7, 2022
Merged

Replace unnecessary use of aiida_profile_clean #5814

merged 2 commits into from
Dec 7, 2022

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Dec 3, 2022

Fixes #5770

@sphuber
Copy link
Contributor Author

sphuber commented Dec 6, 2022

Anyone for this one? @unkcpz @mbercx @ramirezfranciscof ?

@sphuber sphuber force-pushed the fix/replace-unnecessary-aiida-profile-clean branch from 872a631 to 3d8f358 Compare December 6, 2022 09:29
@unkcpz unkcpz self-requested a review December 6, 2022 09:47
unkcpz
unkcpz reviewed Dec 6, 2022
View reviewed changes
Copy link
Member

@unkcpz unkcpz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Not finished yet. I'll continue after the group meeting.
A quick question, after this PR, how long it will save to run all tests? The total time compare with CI tests of other PR, the tests of 3.11 is fast but not 3.8.
If I understand correctly, there is only one profile during a pytest?

tests/cmdline/commands/test_group_ls.py Outdated
@@ -19,8 +19,9 @@


@pytest.fixture
def setup_groups(aiida_profile_clean):
def setup_groups():
Copy link
Member

Choose a reason for hiding this comment

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

Here it seems more clear to use aiida_profile_clean.

@sphuber
Copy link
Contributor Author

sphuber commented Dec 6, 2022

A quick question, after this PR, how long it will save to run all tests?

Unfortunately the run time of the tests on Github Actions are not very stable so it is difficult to say with certainty what the exact speed up is. But the tests of this PR ran in 13m 37s and 13m 27s. For comparison, the latest run on the main branch ran in 15m 5s and 18m 3s. So there seems to be a significant speedup as I would expect, but it can vary.

If I understand correctly, there is only one profile during a pytest?

Mostly yes. Certain tests may create an additional profile on the fly for just that test.

unkcpz
unkcpz requested changes Dec 6, 2022
View reviewed changes
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

@sphuber Thanks! Just some minor requests.

tests/orm/test_querybuilder.py Show resolved Hide resolved
tests/tools/archive/orm/test_codes.py Show resolved Hide resolved
@sphuber
Tests: Replace all use of aiida_profile_clean with aiida_profile
a4e12b4 
The `aiida_profile_clean` will clean the storage backend, which often is
an expensive operation. For the `PsqlDosBackend` for example, this means
dropping all database tables and deleting the file repository before
recreating a clean instance of both from scratch.

Often tests just require a profile to be loaded but it doesn't have to
be clean. The tests can be sped up significantly by only using the
`aiida_profile_clean` fixture where it is really necessary.
@sphuber sphuber force-pushed the fix/replace-unnecessary-aiida-profile-clean branch from 3d8f358 to be9f2a8 Compare December 6, 2022 21:18
@sphuber sphuber requested a review from unkcpz December 6, 2022 21:18
unkcpz
unkcpz approved these changes Dec 6, 2022
View reviewed changes
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Looks all good to me. Cheers!

@sphuber sphuber force-pushed the fix/replace-unnecessary-aiida-profile-clean branch from be9f2a8 to 1334c52 Compare December 6, 2022 21:58
@sphuber sphuber merged commit f3e1968 into aiidateam:main Dec 7, 2022
@sphuber sphuber deleted the fix/replace-unnecessary-aiida-profile-clean branch December 7, 2022 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@unkcpz unkcpz unkcpz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove the unnecessary use of aiida_profile_clean
2 participants
@sphuber @unkcpz