Migrate `Transport` to use pydantic for configuration #6198

sphuber · 2023-11-30T10:04:03Z

Following #6255, we introduce here pydantic models also to the Transport module for use in configuring computers.

agoscinski · 2024-05-29T09:58:22Z

Is it worth to tag this with a v3.0.0 milestone where we could introduce such breaking changes?

sphuber · 2024-05-29T11:44:52Z

Is it worth to tag this with a v3.0.0 milestone where we could introduce such breaking changes?

The breaking changes are actually quite minimal. I don't remember by heart, but it may be just the order in which options are prompted for and some other minimal things. Mostly the changes are just in implementation.

And I am not sure if there actually ever will be a v3 😅 There are no concrete plans for now in any case. But definitely worth a discussion at some point perhaps.

codecov · 2024-05-30T10:06:20Z

Codecov Report

Attention: Patch coverage is 94.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 79.16%. Comparing base (fb723a9) to head (ac9bd11).

Files with missing lines	Patch %	Lines
src/aiida/transports/plugins/ssh.py	91.90%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6198      +/-   ##
==========================================
+ Coverage   79.13%   79.16%   +0.04%     
==========================================
  Files         565      565              
  Lines       43391    43428      +37     
==========================================
+ Hits        34331    34376      +45     
+ Misses       9060     9052       -8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

edan-bainglass · 2025-05-07T05:10:59Z

@agoscinski @khsrali working on this one. Rebased on main. Possibly need to add Models to the new blocking/async Transport classes (or maybe not? I don't see any new fields - @khsrali?). Then tests. Then we should be good. Let's see.

edan-bainglass · 2025-05-07T05:18:29Z

@agoscinski do we want to add model testing for transport similar to that of the orm module?

edan-bainglass · 2025-05-07T05:47:16Z

@agoscinski maybe check that last commit regarding the non-interactive option for the localhost config. I guess this wasn't an issue before due to loose code but now is due to Seb's updates. We use --non-interactive when configuring the slurm-ssh computer, so this change seemed consistent.

Update

Adding --non-interactive didn't take. Environment setup still fails due to missing non-interactive argument. Have a look 🙏

khsrali · 2025-05-07T08:17:05Z

@agoscinski @khsrali working on this one. Rebased on main. Possibly need to add Models to the new blocking/async Transport classes (or maybe not? I don't see any new fields - @khsrali?). Then tests. Then we should be good. Let's see.

https://github.com/aiidateam/aiida-core/blob/71fc14f3c2a501ff8d704d20df76a297edc8e8bc/src/aiida/transports/plugins/ssh_async.py#L70C1-L109C1

edan-bainglass · 2025-05-09T07:07:41Z

@khsrali can you give a look to the new AsyncSshTransport.Model? Note the erroneous use of _DEFAULT_max_io_allowed in the Model. I'm not sure how to reference this class variable within the model. The easy way around is to lift it to a module variable. If you approve, I'll update the PR.

edan-bainglass · 2025-05-15T14:18:45Z

@khsrali please review 🙏

khsrali · 2025-05-15T19:42:41Z

I apologize @edan-bainglass

This was a super busy week.. now I'm out on a vacation, and don't have my laptop with me..

It's quite painful to review with my phone 🥲

Will be back to work on Tuesday the 20th.
I hope that's not too late. If you urgently need this, maybe another team member can review?

P.S. also Alex is on vacation, so I think this can wait from the release point of view.

khsrali

I haven't check the changes in cmd_computer, carefully.
I have to check if we have complete test suits for that.

khsrali · 2025-05-21T11:47:04Z

src/aiida/cmdline/commands/cmd_computer.py

    echo.echo_success(f'Computer `{label}` {"and all its associated nodes " if associated_nodes_pk else ""}deleted.')


-class LazyConfigureGroup(VerdiCommandGroup):


How this change is related?

Part of @sphuber's original work. It seems now more standard w.r.t the way the code command is handled, which was already using pydantic I think since the abstract code refactoring.

I'm not sure why this is the case, but re-instating the LazyConfigureGroup class breaks the TestVerdiComputerConfigure.test_ssh_interactive test. The values passed to the CLI command are somehow out of order and redundant, leading to the following nonsense:

Report: enter ? for help. Report: enter ! to ignore the default and set no value. User name [edanb]: Port number [22]: Look for keys [Y/n]: some_remote_user <- ? Error: invalid input Look for keys [Y/n]: 345 Error: invalid input Look for keys [Y/n]: no SSH key file []: Connection timeout in s [60]: Allow ssh agent [Y/n]: SSH proxy jump []: SSH proxy command []: Compress file transfers [Y/n]: GSS auth [False]: GSS kex [False]: GSS deleg_creds [False]: GSS host [localhost]: Load system host keys [Y/n]: Key policy (RejectPolicy, WarningPolicy, AutoAddPolicy) [RejectPolicy]: Use login shell when executing command [Y/n]: Connection cooldown time (s) [30.0]: Report: Configuring computer test_ssh_interactive for user test@localhost. Success: test_ssh_interactive successfully configured for test@localhost

instead of the correct

Report: enter ? for help. Report: enter ! to ignore the default and set no value. Use login shell when executing command [Y/n]: Connection cooldown time (s) [30.0]: User name [edanb]: some_remote_user Port number [22]: 345 Look for keys [Y/n]: no SSH key file []: Connection timeout (s) [60]: Allow ssh agent [y/N]: SSH proxy jump []: SSH proxy command []: Compress file transfers [Y/n]: GSS auth [y/N]: GSS kex [y/N]: GSS deleg_creds [y/N]: GSS host []: Load system host keys [Y/n]: Key policy (RejectPolicy, WarningPolicy, AutoAddPolicy) [RejectPolicy]: Report: Configuring computer test_ssh_interactive for user test@localhost. Success: test_ssh_interactive successfully configured for test@localhost

At the moment, it's not clear what broke this part. I suppose @sphuber encountered this and fixed it. Unfortunately (uncharacteristically) not documented 😞

Okay, I got it. Digging into the run_cli_command fixture, I see that the user_input argument (a string providing user input to the tested command) can include new line characters to simulate the interactive prompt, i.e., pressing enter to accept defaults. This was changed in the original PR to support the CLI changes - two initial \n lead to accepting the default user name and port, which in turn assigns the user_input username and port to the wrong fields. Removing these passes the test 👍

khsrali · 2025-05-22T07:50:50Z

src/aiida/transports/plugins/ssh_async.py

        ),
    ]

+    class Model(AsyncTransport.Model):


Why are the previous stuff are also kept?
Line 75-110

As discussed, this is unrelated. The "previous stuff" is used in the CLI. However, still to be discussed is if that should remain, or if we use the model. If possible, good if we don't mess with the current UX, which informs the user of issues when they occur. The use of pydantic models will likely defer notification of issues until the end of the CLI process. Not ideal or friendly.

khsrali · 2025-05-22T07:51:10Z

src/aiida/transports/transport.py

        ),
    ]

+    class Model(BaseModel):


The same question here

khsrali · 2025-05-22T07:52:54Z

.github/workflows/setup.sh

 # set up localhost computer
 verdi computer setup --non-interactive --config "${CONFIG}/localhost.yaml"
-verdi computer configure core.local localhost --config "${CONFIG}/localhost-config.yaml"
+verdi computer configure core.local localhost --non-interactive --config "${CONFIG}/localhost-config.yaml"


I wonder why this was not needed before

Update:
@edan-bainglass reports he cannot configure a computer even with this change

That could also potentially mean that, the tests we have in place are not adequate to capture that failure.

@edan-bainglass can you please tailor the error you get?
Thanks!

In the original PR, the configure_computer function in cmd_computer.py expected a non_interactive positional argument. It is unclear why @sphuber had added this. It is not used in the body of the function. Also, in other similar functions (see cmd_code.py), the non_interactive option is wrapped up in kwargs and is not explicitly defined as a positional argument. Removing the non_interactive argument from configure_computer resolves the issue I was encountering.

As for --non-interactive not being included in the original command referenced in this comment, that is unclear. It was included when configuring the slurm computer.

edan-bainglass · 2025-05-23T11:45:41Z

@sphuber I pinged you on at least one comment regarding the original work. However, it would be great if you could provide a description to this PR briefly detailing the motivation and summarizing the changes.

…onsistency There are no additional fields. This is just a reference to the parent `Transport.Model`. Still best to follow the hierarchical chain for consistency.

edan-bainglass · 2025-06-26T05:03:35Z

@khsrali @agoscinski we need to revisit the test_ssh_interactive test. It is failing, but in practice, configuring a computer interactively works. Perhaps a bad test. I'm looking into it.

Update

All good. I found the cause of the failure.

However, I also found test_show_limit to be a fragile test, inconsistently passing/failing the following assert:
assert not (str(nodes[0].pk) in result.output and str(nodes[1].pk) in result.output)
Might be an artifact of running the test locally. Not sure.

agoscinski · 2025-06-26T05:35:15Z

@khsrali can you look at this, I have the feeling it is a related bug as you fixed it in #6919. It looks from a quick glance that the CLI option are now created differentl only by defining a pydantic model

aiida-core/src/aiida/cmdline/groups/dynamic.py

Line 158 in 6b85f04

if not hasattr(cls, 'Model'):

.

edan-bainglass · 2025-06-26T05:36:53Z

@khsrali can you look at this, I have the feeling it is a related bug as you fixed it in #6919. It looks from a quick glance that the CLI option are now created differentl only by defining a pydantic model

aiida-core/src/aiida/cmdline/groups/dynamic.py

Line 158 in 6b85f04

if not hasattr(cls, 'Model'):

.

@agoscinski note that in this PR, I no longer use the dynamic class. Using the original lazy class, with the dynamic class use pushed to another PR (soon to be opened).

edan-bainglass · 2025-06-26T13:32:54Z

@agoscinski @khsrali I think perhaps we were wrong to remove the changes to the CLI. Unlike the ORM pydantic models introduced in #6842, this PR introduces pydantic models to the Transport classes solely for use in the CLI. Not sure if @sphuber expected more of them (e.g., serialization), but in this PR, no such mechanism is introduced. The serialization mechanics introduced in #6842 are limited to Entity subclasses, of which the Transport module is not a part of. So there is no straight forward (de)serialization here.

If the above is accurate, I would close #6924 and reintroduce the CLI changes here, then add a test that the model is operational, whatever that means.

Comments?

edan-bainglass · 2025-08-18T04:56:18Z

@agoscinski @khsrali I think perhaps we were wrong to remove the changes to the CLI. Unlike the ORM pydantic models introduced in #6842, this PR introduces pydantic models to the Transport classes solely for use in the CLI. Not sure if @sphuber expected more of them (e.g., serialization), but in this PR, no such mechanism is introduced. The serialization mechanics introduced in #6842 are limited to Entity subclasses, of which the Transport module is not a part of. So there is no straight forward (de)serialization here.

If the above is accurate, I would close #6924 and reintroduce the CLI changes here, then add a test that the model is operational, whatever that means.

Comments?

@khsrali let's discuss this soon, if you have time. I'd like to close this and resolve the pydantic work.

sphuber force-pushed the fix/transport-model-pydantic branch 2 times, most recently from 6f4e549 to 0909304 Compare January 11, 2024 09:51

sphuber force-pushed the fix/transport-model-pydantic branch 3 times, most recently from c30af1f to d606a83 Compare March 13, 2024 09:40

sphuber mentioned this pull request May 27, 2024

Simplify the interface of verdi computer setup #4981

Open

agoscinski mentioned this pull request May 29, 2024

Inconsistent usage of setup/create/configure in verdi commands profile/user/code/computer to make new instances, set and update parameters #6431

Open

sphuber force-pushed the fix/transport-model-pydantic branch from d606a83 to aa42fd7 Compare May 30, 2024 09:35

edan-bainglass self-assigned this Feb 6, 2025

khsrali self-requested a review April 10, 2025 13:15

agoscinski mentioned this pull request Apr 29, 2025

Pydantic models for serialization #6842

Open

7 tasks

agoscinski removed this from aiida-core v2.7.0 May 2, 2025

agoscinski added this to aiida-core v2.8.0 May 2, 2025

edan-bainglass force-pushed the fix/transport-model-pydantic branch from 0c5f715 to 1d0fab2 Compare May 7, 2025 05:05

edan-bainglass force-pushed the fix/transport-model-pydantic branch from 911b43e to 9bcf7e7 Compare May 9, 2025 07:06

edan-bainglass force-pushed the fix/transport-model-pydantic branch 3 times, most recently from c109b61 to 1965318 Compare May 15, 2025 06:09

khsrali reviewed May 22, 2025

View reviewed changes

sphuber and others added 5 commits June 25, 2025 08:38

WIP: Migrate Transport to use pydantic for configuration

ab723ee

Update SshTransport.Model parent to BlockingTransport.Model for c…

4d2c5c0

…onsistency There are no additional fields. This is just a reference to the parent `Transport.Model`. Still best to follow the hierarchical chain for consistency.

Make localhost configure non-interactive during CI environment setup

e9e211b

Add AsyncSshTransport.Model

c40a195

Lift _DEFAULT_max_io_allowed to module level

8662e9c

edan-bainglass force-pushed the fix/transport-model-pydantic branch from 401dee6 to d93c679 Compare June 25, 2025 06:47

edan-bainglass force-pushed the fix/transport-model-pydantic branch from 6b85f04 to 30596b6 Compare June 26, 2025 05:37

edan-bainglass mentioned this pull request Jun 26, 2025

Update the computer_configure CLI command #6924

Draft

edan-bainglass added 2 commits June 26, 2025 11:37

Discard non-interactive positional argument in configure_computer

745fefe

Fix AsyncSshTransport.Model

dc17aaa

edan-bainglass force-pushed the fix/transport-model-pydantic branch 3 times, most recently from b23417c to 3d8d1fb Compare June 26, 2025 09:44

Revert unrelated changes to the CLI

ac9bd11

edan-bainglass force-pushed the fix/transport-model-pydantic branch from 3d8d1fb to ac9bd11 Compare June 26, 2025 09:47

		echo.echo_success(f'Computer `{label}` {"and all its associated nodes " if associated_nodes_pk else ""}deleted.')


		class LazyConfigureGroup(VerdiCommandGroup):

Migrate Transport to use pydantic for configuration #6198

Are you sure you want to change the base?

Migrate Transport to use pydantic for configuration #6198

Uh oh!

Conversation

sphuber commented Nov 30, 2023 • edited by edan-bainglass Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agoscinski commented May 29, 2024

Uh oh!

sphuber commented May 29, 2024

Uh oh!

codecov bot commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

edan-bainglass commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edan-bainglass commented May 7, 2025

Uh oh!

edan-bainglass commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Update

Uh oh!

khsrali commented May 7, 2025

Uh oh!

edan-bainglass commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edan-bainglass commented May 15, 2025

Uh oh!

khsrali commented May 15, 2025

Uh oh!

khsrali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edan-bainglass Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edan-bainglass commented May 23, 2025

Uh oh!

edan-bainglass commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Update

Uh oh!

agoscinski commented Jun 26, 2025

Uh oh!

edan-bainglass commented Jun 26, 2025

Uh oh!

edan-bainglass commented Jun 26, 2025

Uh oh!

edan-bainglass commented Aug 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Migrate `Transport` to use pydantic for configuration #6198

Migrate `Transport` to use pydantic for configuration #6198

sphuber commented Nov 30, 2023 •

edited by edan-bainglass

Loading

codecov bot commented May 30, 2024 •

edited

Loading

edan-bainglass commented May 7, 2025 •

edited

Loading

edan-bainglass commented May 7, 2025 •

edited

Loading

edan-bainglass commented May 9, 2025 •

edited

Loading

edan-bainglass Jun 25, 2025 •

edited

Loading

edan-bainglass commented Jun 26, 2025 •

edited

Loading