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

🔧 Add types for DefaultFieldsAttributeDict subclasses #6059

Merged
merged 5 commits into from
Jun 17, 2023

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Jun 15, 2023

Improves type checking and LSP completion

(in some sense this a non-breaking alternative to changing them to dataclasses)

Improves type checking and LSP completion
@chrisjsewell chrisjsewell requested a review from sphuber June 15, 2023 11:13
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks. How breaking would it be if we change all these extendeddicts variants to dataclasses? I don't think that plugins really rely on the "additional" functionality of some of them, such as the defaultkeys and extrakeys methods of the DefaultFieldsAttributeDict class. I think we should start the process of deprecating these with the intention to replace them with dataclasses. And then we decide when we actually remove them. I can do some tests with aiida-quantumespresso to see if anything would be affected, but I think very little, if anything should break. What do you think?

@chrisjsewell
Copy link
Member Author

What do you think?

Yeh I think plugins could break; mainly scheduler plugins though, which obviously won't be checked by aiida-quantumespresso. Also serialisation to checkpoints is the other consideration.

I think let's merge this as a quick incremental step, then it shouldn't be to hard to try converting to dataclasses

@sphuber
Copy link
Contributor

sphuber commented Jun 16, 2023

mainly scheduler plugins though, which obviously won't be checked by aiida-quantumespresso.

Yeah fair enough, but there we will have the scheduler plugins in aiida-core to get a sense of impact.

Also serialisation to checkpoints is the other consideration.

Our checkpoint serialization already has a representer for dataclasses, so that should be fine. Of course if people upgrade when they have unfinished processes, then when they restart and they get deserialized, that might cause problems. But that has never been supported. We explicitly tell people to finish any running processes before upgrading.

That reminds me that we cannot deprecate/remove AttributeDict probably as that is actively used a lot, for example by the Process.inputs and WorkChain.ctx. So we might have to keep that one around. But that is ok. It is mostly the other derivative FixedFieldsAttributeDict and DefaultFieldsAttributeDict that we want to get rid off.

@sphuber sphuber merged commit afed5dc into aiidateam:main Jun 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