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.

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

🔃 REFACTOR: Move Scheduler/Transport behind Client API #5901

Closed
wants to merge 14 commits into from

Conversation

chrisjsewell
Copy link
Member

Move API behind ClientProtocol interface, which "holds" both the Transport and Scheduler.

This means that the Scheduler/Transport APIs remain completely unchanged, but it allows for other plugins (such as FireCrest) to to implement the single client API, to do both jobs

Move API behind `ClientProtocol` interface, which "holds" both the Transport and Scheduler.

This means that the Scheduler/Transport APIs remain completely unchanged, but it allows for other plugins (such as FireCrest) to to implement the single client API, to do both jobs
@chrisjsewell chrisjsewell changed the title 🔃 REFACTOR: Scheduler/Transport 🔃 REFACTOR: Move Scheduler/Transport behind Client API Feb 23, 2023
@chrisjsewell
Copy link
Member Author

Ok it's rough around the edges (see e.g. the TODO comments that are the only thing failing the pre-commit: https://github.com/aiidateam/aiida-core/actions/runs/4338703386/jobs/7575599288),
but it does pass all tests 🎉

What does it do:

  • It DOES NOT change Transport or Scheduler in any way, all these plugins are still valid
  • It puts all "internal" interactions with scheduler/transport behind a singular ComputeClientProtocol
  • It implements a single, currently hard-coded, ComputeClientXY class which is initialized with a scheduler+transport
  • it DOES NOT allow for a client plugin yet and/or change the "user facing" CLI/API; that is for another PR
  • It kinda breaks aiida.calculations.monitors plugins, changing their signature from func(computer, transport) to func(computer, client). That is pretty much a necessity, there's no way of getting round it
  • It moves most of the code from aiida.cmdline.commands.computer_test into a ComputeClientProtocol.validate_connection method, so that it can be client dependent
  • Lots of type annotating!

Some things that could potentially be extracted to separate PRs:

  • It updates mypy and pylint, because there were some annoying bugs coming from the current versions

  • For most DefaultFieldsAttributeDict, which are not inherently great for typing (and could probably be replaced with TypedDict/dataclasses), I added a little typing trick, e.g.

     class Subclass(DefaultFieldsAttributeDict):
     
         _default_fields = ( 'x', )
     
         if t.TYPE_CHECKING:
             x: None | int

    so you get typing, but no runtime behaviour is changed

So yes cc @giovannipizzi @sphuber @ltalirz etc, if you wanna have a look and initial comments (just don't get too hung up on module/class names 😅 )

This commit firstly reworks the protocol, to split off methods that require a connected client into `ComputeClientOpenProtocol`, this does not change the concrete implementation, but it improves type checking  and makes it clearer in the code base where open clients are expected.
Some pointers were taking from https://mypy.readthedocs.io/en/latest/protocols.html#defining-subprotocols-and-subclassing-protocols

Once this was done, then all classmethods were changed to standard (instance) method.
The class methods were unnecessary, given that instantiation of an instance has no side-effects, and presented issues for ComputeClientXY (the transport/scheduler combined class)
@chrisjsewell
Copy link
Member Author

Some changes, as per the commit message in bcfdf4b:

This commit firstly reworks the protocol interface a bit, to split off methods that require a connected client into ComputeClientOpenProtocol, this does not change the concrete implementation (thats still a single class), but it improves type checking and makes it clearer in the code base where open clients are expected.
Some pointers were taken from https://mypy.readthedocs.io/en/latest/protocols.html#defining-subprotocols-and-subclassing-protocols

Then also all classmethods were changed to standard (instance) method.
The class methods were unnecessary, given that instantiation of an instance has no side-effects, and presented issues for ComputeClientXY (the transport/scheduler combined class)

@chrisjsewell
Copy link
Member Author

(p.s. stirred a big conversation about protocols during this 😅 python/cpython#102433, python/typing#1363)

@sphuber
Copy link
Contributor

sphuber commented Sep 17, 2023

Closing this as it is superseded by #6043

@sphuber sphuber closed this Sep 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants