👌 IMPROVE: Configuration migrations #5319

chrisjsewell · 2022-01-19T15:54:50Z

Allow for both upgrades and downgrades, and expose this in the CLI.

This will allow a route for user to downgrade their configuration,
if they try a new version of aiida, but then want to return to an old version.

Allow for both upgrades and downgrades, and expose this in the CLI. This will allow a route for user to downgrade their configuration, if they try a new version of aiida, but then want to return to an old version.

chrisjsewell · 2022-01-19T15:57:46Z

I haven't added tests for the CLI commands yet @sphuber; will do if you agree they make sense in general

Note, this is a prelude to adding a migration, to "compartmentalize" config required by a backend (AIIDADB_NAME, etc) to a "storage" key

codecov · 2022-01-19T16:26:27Z

Codecov Report

Merging #5319 (e34d041) into develop (4cf9d93) will increase coverage by 0.03%.
The diff coverage is 94.64%.

@@             Coverage Diff             @@
##           develop    #5319      +/-   ##
===========================================
+ Coverage    82.11%   82.13%   +0.03%     
===========================================
  Files          534      533       -1     
  Lines        38398    38480      +82     
===========================================
+ Hits         31526    31603      +77     
- Misses        6872     6877       +5

Flag	Coverage Δ
django	`77.22% <94.64%> (+0.04%)`	⬆️
sqlalchemy	`76.51% <94.64%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
aiida/manage/__init__.py	`100.00% <ø> (ø)`
aiida/manage/configuration/__init__.py	`83.60% <ø> (ø)`
aiida/manage/configuration/migrations/__init__.py	`100.00% <ø> (ø)`
...iida/manage/configuration/migrations/migrations.py	`94.25% <94.08%> (+0.63%)`	⬆️
aiida/cmdline/commands/cmd_config.py	`92.75% <100.00%> (+0.86%)`	⬆️
aiida/manage/configuration/config.py	`89.18% <100.00%> (ø)`
aiida/transports/plugins/local.py	`81.46% <0.00%> (-0.25%)`	⬇️

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 4cf9d93...e34d041. Read the comment docs.

sphuber · 2022-01-19T16:57:07Z

Cheers @chrisjsewell , guess my first question would be what the advantage is of having this a manual operation? Why shouldn't we just do this automatically? Is the reason because certain downgrades may be lossy? But even in that case, if they want to downgrade, they are forced to migrate anyway, and there will be an automatic backup. So maybe an automatic migration is still the easiest for users?

chrisjsewell · 2022-01-19T17:00:48Z

@sphuber how can you automatically downgrade using an old version of aiida, that does not have that migration available 😬
The only way to downgrade, is to use the version of aiida, that has the upgrade. Unless I'm missing something

sphuber · 2022-01-19T17:11:29Z

@sphuber how can you automatically downgrade using an old version of aiida, that does not have that migration available grimacing The only way to downgrade, is to use the version of aiida, that has the upgrade. Unless I'm missing something

I thought the goal of the downgrade was the following scenario:

User has aiida-core==1.0 with config v1 installed
User upgrades to aiida-core==2.0 which has config v2. Currently, this is automatically upgraded.
User downgrades again to aiida-core==1.0 and now AiiDA will say the config is not compatible

I thought that your description described this scenario, and this would now allow the user to call verdi config downgrade to make their config file compatible again. But my question is whether that should be a manual action or whether we should simply automatically downgrade to the compatible version, just as we upgrade it manually. Why would a user ever want to manually down or upgrade to a version that then won't be compatible with the installation anyway?

Edit: nevermind, I just got the point. Ok, so then we should provide clear instructions (which we can now only do in v2.0 or we need to backport it to v1.6 as well) that in order to downgrade, a more recent version of AiiDA should be installed and then call verdi config downgrade VERSION.

chrisjsewell · 2022-01-19T17:14:28Z

User downgrades again to aiida-core==1.0 and now AiiDA will say the config is not compatible

But how can aiida-core v1 downgrade from a future version?

chrisjsewell · 2022-01-19T17:15:27Z

Edit: nevermind, I just got the point...

Yep 👍

sphuber · 2022-01-19T17:17:20Z

One major problem that I see with all this is that in order to really try a new version of AiiDA, they will have to migrate their database and those are not downgradable, so there is no point in downgrading the config because you won't be able to use the profile anyway.

But I guess, the config can hold more than one profile, and non-migrated profiles could then at least still be used again with an older installation. So maybe this can still be useful. Think just that it should be clearly documented that people should not expect to be able to freely move back and forth between versions. Once you move forward and migrate (both config and storage) you are most likely going to be stuck with that version.

chrisjsewell · 2022-01-19T17:20:46Z

What if you upgrade and create a new profile, then want to downgrade and use an older (unmigrated) profile?

sphuber · 2022-01-20T09:21:52Z

What if you upgrade and create a new profile, then want to downgrade and use an older (unmigrated) profile?

Fair. I guess that there are some niche use cases, so I am fine with moving forward with this. Just think we should be very clear up front that the fact that this is here, it isn't possible to always arbitrarily migrate back and forth. Will now review the rest of the PR.

sphuber · 2022-01-19T16:52:24Z

aiida/cmdline/commands/cmd_config.py

+@click.option('--version', type=int, default=None, help='Upgrade to specific version (default current).')
+def verdi_config_upgrade(version, path):
+    """Print a configuration, upgraded to a specific version."""
+    from aiida.manage.configuration import CURRENT_CONFIG_VERSION, get_config_path, upgrade_config


Can we make this the explicit default in the option, or is aiida.manage one of the modules we cannot import top-level because of loading time?

sphuber · 2022-01-19T16:52:48Z

aiida/cmdline/commands/cmd_config.py

+
+
+@verdi_config.command('upgrade')
+@click.argument('path', required=False, default=None, type=click.Path(exists=True, path_type=Path))


Why do we want this to be configurable? There should always be just a single config.json per instance, shouldn't there?

Actually I think I will remove this command since, on calling the command, verdi is already auto-upgrading, rendering it kinda useless.
The main thing is the downgrad command

aiida/cmdline/commands/cmd_config.py

aiida/manage/configuration/migrations/migrations.py

sphuber · 2022-01-20T09:28:24Z

aiida/manage/configuration/migrations/migrations.py

+        config.setdefault('CONFIG_VERSION', {})['OLDEST_COMPATIBLE'] = 0
+
+    def downgrade(self, config: ConfigType) -> None:
+        config.setdefault('CONFIG_VERSION', {})['OLDEST_COMPATIBLE'] = 0


Should the downgrade not remove these keys, since they were added in the upgrade?

sphuber · 2022-01-20T09:29:07Z

aiida/manage/configuration/migrations/migrations.py

+        from uuid import uuid4  # we require this import here, to patch it in the tests
+        for profile in config.get('profiles', {}).values():
+            profile.setdefault('PROFILE_UUID', uuid4().hex)
+        config.setdefault('CONFIG_VERSION', {})['OLDEST_COMPATIBLE'] = 0


Why is this line necessary? That is from the previous migration no?

aiida/manage/configuration/migrations/migrations.py

sphuber · 2022-01-20T09:33:05Z

aiida/manage/configuration/migrations/migrations.py


+    def downgrade(self, config: ConfigType) -> None:
+        config.setdefault('CONFIG_VERSION', {})['OLDEST_COMPATIBLE'] = 3


Again, this is not really a downgrade since it does not undo the upgrade. Could this cause issues if actually loading the downgrade config with an older version of aiida-core?

IMO, a downgrade is not to "undo an upgrade", it is to make it compliant with the lower versions schema.

As mentioned above, in #5320, I have also removed the fact that Profile strips unknown keys.
I think there may even be an open issue about this: someone inadvertently used an older version of aiida, which stripped these keys, then tried to upgrade again and it was no longer working

Fair enough, happy to go ahead with that behavior if it will be guaranteed to work and we assume that behavior from now on

aiida/manage/configuration/migrations/migrations.py

aiida/cmdline/commands/cmd_config.py

chrisjsewell · 2022-01-20T15:41:00Z

it isn't possible to always arbitrarily migrate back and forth

why though? I don't see any reason why not

chrisjsewell · 2022-01-20T15:42:23Z

I think I've answered all your comments @sphuber, there is just the outstanding issue of the verdi config downgrade functionality+testing, will wait on your comments first though

sphuber · 2022-01-20T15:57:37Z

why though? I don't see any reason why not

Maybe in theory it could be possible, but it is not clear that we always can. The config may be more lenient with allowing additional keys, and so having downgrades not being a full undo of an upgrade, but to me it is not clear that with the PostgreSQL database we have the same luxury. Besides, it would mean an even greater development load by forcing us to implement and test the downgrade of each migration, which would also be non-trivial. In any case, the majority of migrations at this moment don't have downgrades, so those would have to be written first. But I really don't think we should go down this road.

chrisjsewell · 2022-01-20T16:14:45Z

the majority of migrations at this moment don't have downgrades

well, I'd not all of the sqlalchemy migrations have at least some form of downgrade, which we rely on for the migration testing (you have to migrate down, before you can migrate up)

but no I'm not suggesting we put much more effort into supporting this

chrisjsewell · 2022-01-21T04:51:19Z

Ok this should be good to go @sphuber

aiida/manage/configuration/migrations/migrations.py

sphuber · 2022-01-21T17:19:55Z

aiida/cmdline/commands/cmd_config.py

+
+@verdi_config.command('downgrade')
+@click.argument('version', type=int)
+@click.option(


Correct me if I am wrong, but I don't think you answered my question yet why we need the option to specify an arbitrary input and output file. Since this is user facing it should simply always operate on the currently configured config file. I would suggest to remove this additional complexity since I don't think it is needed for now and you don't have tests for it.

sphuber · 2022-01-21T17:20:38Z

aiida/cmdline/commands/cmd_config.py

+
+
+@verdi_config.command('downgrade')
+@click.argument('version', type=int)


Can we make this a type=click.Choice and get the available numbers dynamically from the available migrations? That would make it a lot better from a UI perspective.

aiida/manage/configuration/migrations/migrations.py

sphuber

Thanks @chrisjsewell . I am approving so we can merge this, but I did still wanted to note that because the writing of the config in the downgrade command now goes outside of the Config class, there will be no automatic backup created. Maybe this is something we want to add in the future to prevent losing a configuration if the downgrade were to ever be incorrect.

Allow for both upgrades and downgrades, and expose downgrading in the CLI. This will allow a route for users to downgrade their configuration, if they try a new version of aiida-core, but then want to return to an old version.

👌 IMPROVE: Configuration migration

c5e9d19

Allow for both upgrades and downgrades, and expose this in the CLI. This will allow a route for user to downgrade their configuration, if they try a new version of aiida, but then want to return to an old version.

chrisjsewell requested a review from sphuber January 19, 2022 15:54

Merge branch 'develop' into storage-config

5f64ec1

chrisjsewell changed the title ~~👌 IMPROVE: Configuration migration~~ 👌 IMPROVE: Configuration migrations Jan 19, 2022

sphuber requested changes Jan 20, 2022

View reviewed changes

chrisjsewell mentioned this pull request Jan 20, 2022

♻️ REFACTOR: Profile storage backend configuration #5320

Merged

chrisjsewell commented Jan 20, 2022

View reviewed changes

aiida/cmdline/commands/cmd_config.py Outdated Show resolved Hide resolved

chrisjsewell added 2 commits January 20, 2022 16:30

address review comments

e483a74

Merge branch 'develop' into storage-config

fc142ab

chrisjsewell requested a review from sphuber January 20, 2022 15:41

chrisjsewell added 3 commits January 21, 2022 04:58

add new line between class attributes

16a2703

Update verdi_config_downgrade and add test

e3c059a

Merge branch 'develop' into storage-config

532f315

sphuber requested changes Jan 21, 2022

View reviewed changes

fix docstrings

bd3a6df

chrisjsewell added 6 commits January 21, 2022 18:57

Add failure tests

baea9ce

add typing

4b797eb

Update migrations.py

2f9fa47

remove options

0cfae86

add choice

8e4b70a

Merge branch 'develop' into storage-config

e34d041

chrisjsewell requested a review from sphuber January 21, 2022 18:16

sphuber approved these changes Jan 21, 2022

View reviewed changes

chrisjsewell merged commit 472bff5 into aiidateam:develop Jan 21, 2022

chrisjsewell deleted the storage-config branch January 21, 2022 19:06

chrisjsewell mentioned this pull request Mar 12, 2022

🔀 MERGE: Release v2.0.0b1 #5426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

👌 IMPROVE: Configuration migrations #5319

👌 IMPROVE: Configuration migrations #5319

chrisjsewell commented Jan 19, 2022

chrisjsewell commented Jan 19, 2022

codecov bot commented Jan 19, 2022 •

edited

Loading

sphuber commented Jan 19, 2022

chrisjsewell commented Jan 19, 2022

sphuber commented Jan 19, 2022 •

edited

Loading

chrisjsewell commented Jan 19, 2022

chrisjsewell commented Jan 19, 2022

sphuber commented Jan 19, 2022 •

edited

Loading

chrisjsewell commented Jan 19, 2022

sphuber commented Jan 20, 2022

sphuber Jan 19, 2022

sphuber Jan 19, 2022

chrisjsewell Jan 20, 2022

sphuber Jan 20, 2022

sphuber Jan 20, 2022

sphuber Jan 20, 2022

chrisjsewell Jan 20, 2022

sphuber Jan 20, 2022

chrisjsewell commented Jan 20, 2022

chrisjsewell commented Jan 20, 2022

sphuber commented Jan 20, 2022

chrisjsewell commented Jan 20, 2022 •

edited

Loading

chrisjsewell commented Jan 21, 2022

sphuber Jan 21, 2022

chrisjsewell Jan 21, 2022

sphuber Jan 21, 2022

chrisjsewell Jan 21, 2022

sphuber left a comment



		@verdi_config.command('upgrade')
		@click.argument('path', required=False, default=None, type=click.Path(exists=True, path_type=Path))


		def downgrade(self, config: ConfigType) -> None:
		config.setdefault('CONFIG_VERSION', {})['OLDEST_COMPATIBLE'] = 3



		@verdi_config.command('downgrade')
		@click.argument('version', type=int)

👌 IMPROVE: Configuration migrations #5319

👌 IMPROVE: Configuration migrations #5319

Conversation

chrisjsewell commented Jan 19, 2022

chrisjsewell commented Jan 19, 2022

codecov bot commented Jan 19, 2022 • edited Loading

Codecov Report

sphuber commented Jan 19, 2022

chrisjsewell commented Jan 19, 2022

sphuber commented Jan 19, 2022 • edited Loading

chrisjsewell commented Jan 19, 2022

chrisjsewell commented Jan 19, 2022

sphuber commented Jan 19, 2022 • edited Loading

chrisjsewell commented Jan 19, 2022

sphuber commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell commented Jan 20, 2022

chrisjsewell commented Jan 20, 2022

sphuber commented Jan 20, 2022

chrisjsewell commented Jan 20, 2022 • edited Loading

chrisjsewell commented Jan 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 19, 2022 •

edited

Loading

sphuber commented Jan 19, 2022 •

edited

Loading

sphuber commented Jan 19, 2022 •

edited

Loading

chrisjsewell commented Jan 20, 2022 •

edited

Loading