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

[Enhancement]: Enable static type checking with pyright #34

Open
NucciTheBoss opened this issue Nov 15, 2024 · 0 comments
Open

[Enhancement]: Enable static type checking with pyright #34

NucciTheBoss opened this issue Nov 15, 2024 · 0 comments
Labels
Priority: Low This should be addressed by the end of the cycle/milestone. Type: Enhancement Proposes a new feature to be added to the project.

Comments

@NucciTheBoss
Copy link
Member

slurmutils current doesn't have static type-checking enabled which is fine as it's a Python project, but it's causing issues for downstreams like hpc-libs and slurm-charms which do have static type checking enabled.

The issue is that slurmutils makes heavy use of dynamic attributes for accessing various configuration values such as auth_alt_attributes, auth_type, slurmctld_parameters, etc. This is fine if you're only using Python's duck type system, but static type checkers such as pyright or mypy will complain with the following error:

/home/ubuntu/slurm-charms/_build/slurmdbd/src/charm.py
  /home/ubuntu/slurm-charms/_build/slurmdbd/src/charm.py:246:33 - error: Cannot assign to attribute "auth_alt_types" for class "SlurmdbdConfig"
    Attribute "auth_alt_types" is unknown (reportAttributeAccessIssue)
  /home/ubuntu/slurm-charms/_build/slurmdbd/src/charm.py:247:33 - error: Cannot assign to attribute "auth_alt_parameters" for class "SlurmdbdConfig"
    Attribute "auth_alt_parameters" is unknown (reportAttributeAccessIssue)
2 errors, 0 warnings, 0 informations 

Easy solution

The easiest fix is to just direct the type checker to just simply ignore the error by either commenting # pyright: reportAttributeAccessIssue=false at the top of the problematic Python file, or use the inline comment # pyright: ignore [reportAttributeAccessIssue] for each line that's causing the issue. However, this becomes impractical at scale as # pyright: reportAttributeAccessIssue=false will suppress the error for EVERYTHING, not just the slurmutils config models, and # pyright: ignore [reportAttributeAccessIssue] becomes untenable if you're heavily using slurmutils within your code.

More difficult but worthwhile solution

Unfortunately, pyright finds dynamic attributes to be cringe as they make it difficult to be serious about strict typing: microsoft/pyright#153

There's two options here:

  1. Define "dummy" __getattr__, __setattr__, and __delattr__ that only exist during type checking on all the configuration models. This will instruct pyright that all the models support dynamic attributes which means that downstream consumers won't need to comment # pyright: ignore [reportAttributeAccessIssue] everywhere. I don't love this solution however as it feels hacky as we're effectively just bolting on support for type checking rather than using it to our benefit.

  2. Explicitly declare all properties for each configuration model rather than dynamically generating them. This way we can still get the full benefit of type checking, and we can better alert users if they're inserting a bad value into one of the configuration models. The other, not super important benefit, is that we also get IDE autocompletions. The downside to this approach however is the maintenance burden of maintaining descriptors for every single configuration option we want to support. It will grow the total LoC to be much bigger than what the total LoC is now, and most of the LoC will mostly just be boilerplate for the configuration models.

@NucciTheBoss NucciTheBoss added Priority: Low This should be addressed by the end of the cycle/milestone. Type: Enhancement Proposes a new feature to be added to the project. labels Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low This should be addressed by the end of the cycle/milestone. Type: Enhancement Proposes a new feature to be added to the project.
Projects
None yet
Development

No branches or pull requests

1 participant