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

Mutability of BigQueryReservation confuses type checker #6834

Open
hannes-ucsc opened this issue Jan 20, 2025 · 0 comments
Open

Mutability of BigQueryReservation confuses type checker #6834

hannes-ucsc opened this issue Jan 20, 2025 · 0 comments
Labels
- [priority] Medium code [subject] Production code debt [type] A defect incurring continued engineering cost orange [process] Done by the Azul team

Comments

@hannes-ucsc
Copy link
Member

hannes-ucsc commented Jan 20, 2025

The BigQueryReservation class has mutable instances attributes that are being assigned to at various stages of the control flow. This makes it hard to understand for the reader and the type checker when they are expected to already be initialized and when not. I had to add several redundant assertions to help the type checker along. See FIXME. To see the type checker's confusion first hand, revert the commit that adds the FIXME (except for the hunk that adds the module to .mypy.ini) and run mypy:

src/azul/bigquery_reservation.py:131: error: Item "None" of "Reservation | None" has no attribute "autoscale"  [union-attr]
src/azul/bigquery_reservation.py:133: error: Item "None" of "Reservation | None" has no attribute "autoscale"  [union-attr]
src/azul/bigquery_reservation.py:172: error: Item "None" of "Reservation | None" has no attribute "autoscale"  [union-attr]
src/azul/bigquery_reservation.py:192: error: Item "None" of "Reservation | None" has no attribute "name"  [union-attr]
src/azul/bigquery_reservation.py:193: error: Item "None" of "Reservation | None" has no attribute "name"  [union-attr]
src/azul/bigquery_reservation.py:223: error: Name "Union" is not defined  [name-defined]
src/azul/bigquery_reservation.py:223: note: Did you forget to import it from "typing"? (Suggestion: "from typing import Union")
src/azul/bigquery_reservation.py:229: error: Name "Union" is not defined  [name-defined]
src/azul/bigquery_reservation.py:229: note: Did you forget to import it from "typing"? (Suggestion: "from typing import Union")
src/azul/bigquery_reservation.py:235: error: Variable "azul.bigquery_reservation.BigQueryReservation.ResourcePager" is not valid as a type  [valid-type]
src/azul/bigquery_reservation.py:235: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
src/azul/bigquery_reservation.py:235: error: Name "Optional" is not defined  [name-defined]
src/azul/bigquery_reservation.py:235: note: Did you forget to import it from "typing"? (Suggestion: "from typing import Optional")
src/azul/bigquery_reservation.py:236: error: Need type annotation for "resources" (hint: "resources': list[<type>] = ...")  [var-annotated]
Found 10 errors in 1 file (checked 15 source files)
(.venv) hannes-m1bp:azul.hannes2.local hannes$ 

I think the control and information flow would be clearer if methods like create_reservation returned their result instead of assigning to a mutable instance attribute. The implicit post-condition is that self.reservation is not None when self.create_reservation returns. This post-condition can be made explicit by declaring def create_reservation(self, …) -> Reservation.

@hannes-ucsc hannes-ucsc added the orange [process] Done by the Azul team label Jan 20, 2025
@achave11-ucsc achave11-ucsc added bug debt [type] A defect incurring continued engineering cost code [subject] Production code - [priority] Medium labels Jan 21, 2025
@achave11-ucsc achave11-ucsc removed the bug label Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- [priority] Medium code [subject] Production code debt [type] A defect incurring continued engineering cost orange [process] Done by the Azul team
Projects
None yet
Development

No branches or pull requests

2 participants