Skip to content

chore: migrate types & type hint machine module#1015

Closed
yanksyoon wants to merge 10 commits intojuju:masterfrom
yanksyoon:chore/typing
Closed

chore: migrate types & type hint machine module#1015
yanksyoon wants to merge 10 commits intojuju:masterfrom
yanksyoon:chore/typing

Conversation

@yanksyoon
Copy link
Copy Markdown
Contributor

@yanksyoon yanksyoon commented Jan 27, 2024

Description

This migrates all the known status types from juju https://github.com/juju/juju/blob/f8e67c7f7c16f03312b96a0731a2b3fdbef1645e/core/status/status.go#L69
to the python-libjuju's status.py module.

This helps developers get a first-class support for type hints when using modules from Juju. As a start, machine module has been type-hinted which is useful when using with ops.testing library. After the change has been merged, other modules such as unit, applications and models can further be type hinted.

Some bugs and assumptions can be cleared out with type hints as well as a user.

QA Steps

<Commands / tests / steps to run to verify that the change works:>

tox -e py3 -- tests/unit/...
tox -e integration -- tests/integration/...

All CI tests need to pass.

<Please note that most likely an additional test will be required by the reviewers for any change that's not a one liner to land.>

Notes & Discussion

<Additional notes for the reviewers if needed. Please delete section if not applicable.>

@jujubot
Copy link
Copy Markdown
Contributor

jujubot commented Jan 27, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

1 similar comment
@jujubot
Copy link
Copy Markdown
Contributor

jujubot commented Jan 27, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@tlm
Copy link
Copy Markdown
Member

tlm commented Jan 30, 2024

@yanksyoon thank you for the pull request. I have asked @cderici to have a look as he understands a lot better what is going on. Would you mind filling out your why a bit more for the PR? Are you able to describe a potential problem this is solving for you. Doesn't have to be much just helps any future developers to understand why something change a bit better.

@tlm tlm requested a review from cderici January 30, 2024 05:54
@yanksyoon
Copy link
Copy Markdown
Contributor Author

@tlm Sure! I thought this would be a good step for making dev-exp with Juju better. The charmers rely heavily on the ops library and many of the ops library refers to the python-libjuju components.
Updated the description!

Copy link
Copy Markdown
Contributor

@cderici cderici left a comment

Choose a reason for hiding this comment

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

I think this is a step towards the right direction. Although status in particular is a tricky subject in Juju (I'll make more detailed comments in #1016 about status), introducing type annotations in pylibjuju is certainly useful, as the project is growing larger. However, I'm not a huge fan of introducing the status types like this, because:

  1. I feel like this is an overcomplication of the client, i.e. pylibjuju shouldn't need to keep track of the statuses that Juju maintains, i.e. what happens if Juju 4.0 changes one of these statuses, then we need to add that as a new option, but still need to keep the old statuses to maintain compatibility. Quick glance shows that these didn't change btw 2.9 and 3.x, but that doesn't mean that it won't. So my question mainly is, does pylibjuju really need to know these? Especially because;
  2. status is a dynamic information that we receive from the API during the runtime, and static type checking is, well, static. So we can't statically check if the types (and values) we'll get are valid. A static type checker won't be able to make sure a call to def agent_status(self) -> MachineAgentStatusT: is valid, and that type information is not dynamically enforced by the Python runtime, so not very useful. I think if we do end up introducing the statuses, a better way to use that information would be to assert the types inside the functions before returning, effectively type checking at the runtime ourselves.

I'm a big fan of static typing, however, that's only good for function calls internal to pylibjuju. We can add a type checker (e.g. mypy, or pyre) to our production to make things rock solid, all great, however, any interaction with Juju can't be checked statically, is my main problem here. WDYT?

I'd like to hear @SimonRichardson's thoughts on the (1)st item above.

@yanksyoon
Copy link
Copy Markdown
Contributor Author

@cderici Thanks for the explanation, I feel like that Juju having a compatibility matrix like Kubernetes does for server - client version would help a lot.
Having to run the tests and see what's being returned from juju takes quite a lot of time as a charm developer and having type hints would really reduce the amount of time spent - since Juju docs also is not versioned as of yet.

@cderici
Copy link
Copy Markdown
Contributor

cderici commented Jan 31, 2024

I feel like that Juju having a compatibility matrix like Kubernetes does for server - client version would help a lot.

Are you talking about pylibjuju compatibility with Juju controllers? If that's the case it's quite simple, actually, currently the latest 2.9.x pylibjuju is compatible with latest Juju 2.9, and latest 3.x pylibjuju is compatible with any Juju 3.y. I might add a section to the Readme about that. Let me know if this is different than what you were talking about 👍

Having to run the tests and see what's being returned from juju takes quite a lot of time as a charm developer and having type hints would really reduce the amount of time spent - since Juju docs also is not versioned as of yet.

This is fair. Improving dev experience might be a good reason to add the types. My concern is, that keeping backwards compatibility in pylibjuju will require adding union types everywhere. I think a better solution would be to improve the documentation for that. WDYT? Would it make it equally easier if pylibjuju documentation is improved so that it includes type signatures?

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 3, 2024

This PR is marked as incomplete because it has been open 30 days with no activity. Please remove incomplete label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the state/incomplete need more information label Mar 3, 2024
@github-actions github-actions bot removed the state/incomplete need more information label Mar 5, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 4, 2024

This PR is marked as incomplete because it has been open 30 days with no activity. Please remove incomplete label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the state/incomplete need more information label Apr 4, 2024
@yanksyoon yanksyoon closed this by deleting the head repository Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state/incomplete need more information

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants