Make frozendict generic for type annotation #3641
Comments
dpeng817
added
core
type: troubleshooting
dpeng817
added a commit
that referenced
this issue
Feb 4, 2021
Summary: This adds type annotations to definitions/dependency.py. Note that fxns returning `frozendict` have not been annotated with key and value types, this is due to a pylint issue that is preventing generic frozendict from being a thing: #3641. Test Plan: Unit Tests Reviewers: alangenfeld Reviewed By: alangenfeld Differential Revision: https://dagster.phacility.com/D6082
mrdavidlaing
pushed a commit
to mrdavidlaing/dagster
that referenced
this issue
Feb 20, 2021
Summary: This adds type annotations to definitions/dependency.py. Note that fxns returning `frozendict` have not been annotated with key and value types, this is due to a pylint issue that is preventing generic frozendict from being a thing: dagster-io#3641. Test Plan: Unit Tests Reviewers: alangenfeld Reviewed By: alangenfeld Differential Revision: https://dagster.phacility.com/D6082
This was referenced Feb 12, 2023
smackesey
added a commit
that referenced
this issue
Mar 23, 2023
### Summary & Motivation Internal companion PR: dagster-io/internal#5239 Relevant discussion: dagster-io/internal#4859 (comment) Removes the `frozenlist`, `frozendict` and `frozentags` classes. Reasons: - They don't play nicely with static type-checking and are a source of type-checker diagnostic noise - Static type-checking with Sequence/Mapping sort of solves the same problem (this is kind of like removing runtime check calls from internal methods) - They are used inconsistently in the codebase (most places where we have the intention of creating an immutable list/dict they are not used). Where they are used, it is not at all obvious why at first glance. - They generally complicate the code The main purpose the `frozen*` classes were serving in our code was to make a few select `NamedTuple` classes hashable. If a `NamedTuple` contains mutable collection members, its default hash function will fail. Replacing those mutable collections with immutable ones lets the hash succeed, which in turns lets the `NamedTuple` be cached via `lru_cache` or used as a dict key. The set of classes that need to be made hashable for `lru_cache` purposes are: - `CustomPointer` - `AssetsDefinitionCacheableData` - `RepositoryLoadData` - `ReconstructableRepository` These are part of a tree of objects rooted on `ReconstructablePipeline`, which uses `lru_cache` in its `get_definition` method. The above classes can be made hashable in a more legible and type-annotation-friendly way by defining a `__hash__` method. This PR does that-- wherever we have a `NamedTuple` that needed to be hashed, `frozen*` instantiations in the constructor were removed and a `__hash__` method was added. All of the `__hash__` methods are the same, they just call `dagster._utils.hash_named_tuple` and cache the result. This function just converts mutable collections to tuples prior to hashing, which allows hashing to succeed as tuples are immutable. Aside from cases where frozen classes were used to achieve `NamedTuple` hashability, the other uses were: - random? uses in some places where whoever wrote the code thought the structure should be immutable. I removed these cases for the reasons above (most of the time we want structures to be immutable and in most of the codebase we signal this with `Sequence`/`Mapping`. - A few other cases where dicts need to be hashable to sort them or use them in a cache. For these, I provided simple substitute solutions that achieve the same effect-- e.g. instead of using `frozendict`, converting a dict to a tuple of 2-tuples before hashing. --- - Resolves #3008 - Resolves #3641 ### How I Tested These Changes Existing test suite.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Trying to make frozendict a generic that inherits dictionary causes linting issues.
Seems related to this pylint issue: pylint-dev/pylint#2420
Things that I have tried:
class frozendict(Dict[_KT, _VT], Generic[_KT, _VT]):class frozendict(dict, Mapping[_KT, _VT]):class frozendict(dict, Generic[_KT, _VT]):errors it runs into:
The text was updated successfully, but these errors were encountered: