Add export archive migration for Group type strings

[sphuber]

Fixes #3908

The Group entity class is now subclassable, for which the type string
of existing entries in the database had to be changed through a
migration. Here we add the corresponding migration for existing export
archives. The only required change is to map the type string of groups.

To test this migration we use the existing export archives in the module
tests/fixtures/export/migrate. They did not contain any group entities
so export_v0.1_simple.aiida was updated first and had four groups
added, one for each of the migrated type strings. This initial archive
was then migrated to each subsequent version, step by step, using the
command verdi export migrate --version.

[sphuber]

@CasperWA this PR is blocked because it depends on #3882 and #3903 need to be merged first. But I wanted to already open it to discuss it. We only have to look at the last commit. The migration is relatively simple, just updating the type_string of Group entities in archives. To test this, I use the existing export archives in the tests/fixtures/export/migrate. They did not contain any group entities, so I updated v0.1 to add 4 groups, one of each currently existing type strings, and then migrated it step by step to create each archive for each version (hence my previous PR for `verdi export migrate --version 😉 ).

Question: is this sufficient, or do we need an update in the archives that are hosted in the external repository as well?

[codecov]

Codecov Report

Merging #3912 into develop will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #3912   +/-   ##
========================================
  Coverage    78.21%   78.22%           
========================================
  Files          460      461    +1     
  Lines        34036    34050   +14     
========================================
+ Hits         26621    26635   +14     
  Misses        7415     7415           

Flag	Coverage Δ
#django	70.25% <100.00%> (+0.01%)	⬆️
#sqlalchemy	71.11% <100.00%> (+0.01%)	⬆️

Impacted Files	Coverage Δ
aiida/tools/importexport/common/config.py	100.00% <100.00%> (ø)
aiida/tools/importexport/migration/v08_to_v09.py	100.00% <100.00%> (ø)

---

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 9a75173...08e5310. Read the comment docs.

[sphuber]

@CasperWA do you have an idea when you could give a look at this? Because this PR is blocking the release

[CasperWA]

@CasperWA do you have an idea when you could give a look at this? Because this PR is blocking the release

I can see what time I can spare over Easter and get it done. Otherwise it will not be until after, I'm afraid.

[CasperWA]

Cheers @sphuber.

An initial question:

• What changed in the export_v0.X_simple.aiida archives? They shouldn't change, I believe, right?

And then, let's go through the dedicated wiki-page.
There are 3 major steps:

1. Export/Import archive schema
   For this I see no changes to the import/export config. So all good here.

2. Archive migrations
   This seems to also be all done.

3. Tests

   a. Export/Import tests for changes introduced to export/import

   You may or may not choose to add a small test here (at tests/tools/importexport/orm/test_groups.py) making sure the type_string is as expected. Perhaps a quick look through the tests may also find that some may need updating?

   b. Archive migration tests

   No PR has been made for the aiida-export-migration-tests repository in order to update it to v0.9, with all that it may entail.
   Also, you should add a test_v0.8_to_newest in tests.tools.importexport.migration.text_migration:TestExportFileMigration
   And finally, you're missing a test specifically for the migration_dbgroup_type_string function.

[sphuber]

Thanks a lot for the review @CasperWA , especially during this easter weekend.

An initial question:

* What changed in the `export_v0.X_simple.aiida` archives? They shouldn't change, I believe, right?

As explained in the commit and OP:

To test this migration we use the existing export archives in the module
tests/fixtures/export/migrate. They did not contain any group entities
so export_v0.1_simple.aiida was updated first and had four groups
added, one for each of the migrated type strings. This initial archive
was then migrated to each subsequent version, step by step, using the
command verdi export migrate --version.

So I indeed had to update them all because none of them contained any groups.

And then, let's go through the dedicated wiki-page.

Ah yes, apologies, forgot that that existed.

3. Tests
   a. Export/Import tests for changes introduced to export/import
   You may or may not choose to add a small test here (at `tests/tools/importexport/orm/test_groups.py`) making sure the `type_string` is as expected. Perhaps a quick look through the tests may also find that some may need updating?
   b. Archive migration tests
   No PR has been made for the [`aiida-export-migration-tests` repository](https://github.com/aiidateam/aiida-export-migration-tests) in order to update it to v0.9, with all that it may entail.
   Also, you should add a `test_v0.8_to_newest` in `tests.tools.importexport.migration.text_migration:TestExportFileMigration`
   And finally, you're missing a test specifically for the `migration_dbgroup_type_string` function.

This last part 3b was exactly what I was wondering. Since the changes are really minor (just a changing of the type_string) and those changes are already tested through the simple archives that are included in aiida-core itself, is it really necessary to create an additional archive on aiida-export-migration-tests?

[CasperWA]

To test this migration we use the existing export archives in the module
tests/fixtures/export/migrate. They did not contain any group entities
so export_v0.1_simple.aiida was updated first and had four groups
added, one for each of the migrated type strings. This initial archive
was then migrated to each subsequent version, step by step, using the
command verdi export migrate --version.

Cool, thanks! 👍

3. Tests
   a. Export/Import tests for changes introduced to export/import
   You may or may not choose to add a small test here (at `tests/tools/importexport/orm/test_groups.py`) making sure the `type_string` is as expected. Perhaps a quick look through the tests may also find that some may need updating?
   b. Archive migration tests
   No PR has been made for the [`aiida-export-migration-tests` repository](https://github.com/aiidateam/aiida-export-migration-tests) in order to update it to v0.9, with all that it may entail.
   Also, you should add a `test_v0.8_to_newest` in `tests.tools.importexport.migration.text_migration:TestExportFileMigration`
   And finally, you're missing a test specifically for the `migration_dbgroup_type_string` function.

This last part 3b was exactly what I was wondering. Since the changes are really minor (just a changing of the type_string) and those changes are already tested through the simple archives that are included in aiida-core itself, is it really necessary to create an additional archive on aiida-export-migration-tests?

You can definitely argue that it's overkill.
However, I suspect that skipping a version (no matter the minority of it) will end badly for future migration testing.
I just saw that the aiida-fleur plug-in also relies on the aiida-export-migration-tests package - for testing, I assume - so skipping a version may present unknown problems (in testing), both for aiida-core and others.

In any case, I have created a PR in the external repo that you can go through. Concerning the updated archive, I have only changed the type_string of the only group that was in there and updated the metadata.json file to reflect the new version.

[CasperWA]

You should update to aiida-export-migration-tests==0.9.0 in this PR (just released).

[sphuber]

/update-requirements

[aiida-bot-app]

Starting 'update-requirements' workflow for this branch.

[sphuber]

@CasperWA this is ready for review

[CasperWA]

Much more streamlined now it seems. Cool, thanks @sphuber.
Only a single comment/consideration.

Edit: Also, you may want to make a test under tests/tools/importexport/orm/test_groups.py to make sure the type_strings are correct in the created data.json file.
Again, this is in line with my comment here, and may be overkill and a bit too specific a test?

[sphuber]

Edit: Also, you may want to make a test under tests/tools/importexport/orm/test_groups.py to make sure the type_strings are correct in the created data.json file.
Again, this is in line with my comment here, and may be overkill and a bit too specific a test?

I don't think really understand why you think there should be a test of the migration function in the orm subsection. The migration functionality should be tested in the migration tests. I do agree with your other suggestion though. Even though I made sure that the test archives now contain one group of each type, that may be changed (by mistake) in the future and then it won't be tested anymore. I will add an explicit test for the function

[CasperWA]

Edit: Also, you may want to make a test under tests/tools/importexport/orm/test_groups.py to make sure the type_strings are correct in the created data.json file.
Again, this is in line with my comment here, and may be overkill and a bit too specific a test?

I don't think really understand why you think there should be a test of the migration function in the orm subsection. The migration functionality should be tested in the migration tests. (...)

It is in the importexport orm subsection. And it's not to test the migration, but to test that the export function is writing the correct data for Groups. But again, it may be overkill, as it would suggest we should check this for all the itty-bitty details of Node database column values, which is overkill, I think.

[sphuber]

It is in the importexport orm subsection. And it's not to test the migration, but to test that the export function is writing the correct data for Groups.

I see, except this code has not been touched nor changed, so don't think we should add a test

[sphuber]

@CasperWA this is ready for review

[CasperWA]

Thanks for the extra test @sphuber !

It said the export_v0.9_simple.aiida also changed in the latest commit, did you add or change something?

In any case, I'll approve this PR in its current state. The extra and extremely specific test under importexport/orm/test_groups.py that we have discussed is still open for you to do, but I do not consider it essential, as we should already have simple tests that makes sure we can export and import Groups, which is enough for me, and can be argued is enough for making sure the type_strings are valid, right?

[CasperWA]

It is in the importexport orm subsection. And it's not to test the migration, but to test that the export function is writing the correct data for Groups.

I see, except this code has not been touched nor changed, so don't think we should add a test

No, it is very implicit indeed. I've accepted the PR on the basis of the current changes, but left this open for you to decide upon yourself. I consider it good enough.

[sphuber]

Thanks for the review @CasperWA . I hold off a bit with merging still because technically the tests failed, but that is due to a non-related issue. The pgsu package still is not available on conda. If this won't be resolved today, I will temporarily disable those tests until it is fixed.

In any case, I'll approve this PR in its current state. The extra and extremely specific test under importexport/orm/test_groups.py that we have discussed is still open for you to do, but I do not consider it essential, as we should already have simple tests that makes sure we can export and import Groups, which is enough for me, and can be argued is enough for making sure the type_strings are valid, right?

The type_string is literally stored as a column on the DbGroup table. There is no logic between getting that value to storing it in the export file. Like all other properties of entities, this is a one-to-one dump, so it would really be overkill to test this. That would be like testing that a pk or UUID is exported correctly

[CasperWA]

In any case, I'll approve this PR in its current state. The extra and extremely specific test under importexport/orm/test_groups.py that we have discussed is still open for you to do, but I do not consider it essential, as we should already have simple tests that makes sure we can export and import Groups, which is enough for me, and can be argued is enough for making sure the type_strings are valid, right?

The type_string is literally stored as a column on the DbGroup table. There is no logic between getting that value to storing it in the export file. Like all other properties of entities, this is a one-to-one dump, so it would really be overkill to test this. That would be like testing that a pk or UUID is exported correctly

This was my understanding and thought as well, concerning the "overkill". Great to have it confirmed.