Update type annotations throughout the code base - #1677
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request performs a large-scale refactoring of type annotations across the codebase to align with modern Python standards (PEP 585). Specifically, it replaces deprecated typing module aliases like List, Dict, Tuple, Set, and Type with their built-in lowercase counterparts. Additionally, it migrates abstract collection types such as Iterable, Sequence, Callable, Iterator, and Mapping from typing to collections.abc. These changes are applied consistently across Python source files, development tools, and Jupyter notebooks, resulting in cleaner code and reduced reliance on the typing module. I have no feedback to provide.
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
Thanks for doing this!
This is a monster PR and it's hard to do individual line-oriented comments, so I'll have to summarize change requests and ask you to find the individual instances. Overall it looks good! There are just a few repeated patterns of changes that can be improved:
-
Some of the new
Uniontypes can be rewritten to simpler forms. There are two cases I see:-
A type declaration of the form
Union[X, None]can be rewritten asX | None, but it is safest to do this only if theXis not a quoted type name. For example, the case ofcall_graph_example: Union[BloqExample, None] = field()
can be rewritten as
call_graph_example: BloqExample | None = field()
-
Unions of the form
Union[X, None]are also equivalent toOptional[X], and this is safer to use this variant if theXis a quoted type name. So for example,Union['cirq.Operation', None]
can be converted to
Optional['cirq.Operation']
-
-
Some combinations of Optional and Union can be further simplified. Here is an example:
target_bitsizes: Optional[Union[SymbolicInt, tuple[SymbolicInt, ...]]] = None
can be
target_bitsizes: SymbolicInt | tuple[SymbolicInt, ...] | None = None
-
There are some remaining uses of
typing.Type. I'm looking atqualtran/bloqs/data_loading/qroam_clean.pybut are probably others. Basically, things likecls: Type['QROAMCleanAdjoint']can becls: type['QROAMCleanAdjoint']and the import ofTypefromtypingcan be dropped.
That's all I have for now.
…ections` (#1851) Partial on #1653 As suggested by @mhucka isolate the changes from #1677 to smaller subsets of the codebase so that the review process is easier. This PR removes compatibility with pre-3.10 python versions, but Qualtran isn't intended to target those anymore. --------- Co-authored-by: Michael Hucka <mhucka@google.com>
|
Hi @mhucka Thank you for the detailed feedback, just to confirm that this is in addition to the changes to |
Hi @micpap25 – thanks for coming back to this. Yes, I think that, basically, if there are cases of upper case Set, it should be possible to replace them with the lower-case variant. The tricky cases are what we discussed before about quoted type names. |
|
@mhucka What about using |
mhucka
changed the title
typing aliases to use collections.abc instead
| .astype(int) | ||
| ) | ||
| summary.columns = [v.name.lower() for v in summary.columns] | ||
| summary.columns = [getattr(v, 'name', str(v)).lower() for v in summary.columns] |
| if 'SympySymbolAllocator' not in str(annot.get('ssa', '')): | ||
| print(f"{bc}.build_call_graph `ssa: 'SympySymbolAllocator'`") | ||
| if annot['return'] != Set['BloqCountT']: # type: ignore[misc] | ||
| print(f"{bc}.build_call_graph -> 'BloqCountT'") | ||
| ret_str = str(annot.get('return', '')) | ||
| if not any(sub in ret_str for sub in ('BloqCountDictT', 'set[BloqCountT]', 'Mapping[')): | ||
| print(f"{bc}.build_call_graph -> {ret_str!r}") |
There was a problem hiding this comment.
this goes against the spirit of using the script to check that the annotations are (1) there and (2) the idiomatic ones. I think success for any Mapping[ is particularly loose
| from qualtran.cirq_interop._bloq_to_cirq import _wire_symbol_to_cirq_diagram_info | ||
|
|
||
| if isinstance(self.subbloq, cirq.Gate): | ||
| if isinstance(self.subbloq, GateWithRegisters): |
| raise TypeError("Tried to index into a single soquet.") | ||
|
|
||
| @property | ||
| def bb(self) -> Any: |
There was a problem hiding this comment.
where does this come up?
| if not isinstance(other, Signature): | ||
| return False |
There was a problem hiding this comment.
this is probably right, but it's hard to review this PR which mostly contains respellings of type annotations with sporadic logic changes such as this
|
I started skimming through this PR, but it's very difficult to review. It's 99% "respellings" with no runtime changes whatsoever; but sprinkled with some logic changes that I'd actually like to look at. In particular, adding default values and fallbacks is something I've found to be particularly pernicious in the age of agentic coding when the bots sometimes use it to swallow real errors |
3 participants
Fixes #1653 and in addition, updates all type annotations to modern syntax and removes no-longer-needed
# type: ignorecomments where possible. Takes advantage of constructs available in Python 3.12 (which is the current minimum for Qualtran).This work was started by @micpap25, with subsequent additional work by @mhucka and assistance from Gemini CLI.
General summary:
typeto define types instead ofTypeAliaslistanddictinstead oftyping'sListandDictUnionandOptionalwith shorthand|notation# type: ignorecomments as possibleignore_errorsfrom Mypy configurationwarn_unused_ignores = trueandwarn_redundant_casts = trueto Mypy config