Fix type inference on get_dataframe#69
Closed
damonbayer wants to merge 8 commits into
Closed
Conversation
Collaborator
|
I'll check this out! |
Collaborator
|
this seems to be working for me. It should be good to merge after pulling in the current main branch of cfa-dataops |
Collaborator
|
this problem was fixed in the related PR: #98 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #68.
This is appears to work, but is entirely llm-authored in an area where I lack expertise. Merge with caution.
LLM Explanation
The changes in catalog.py are only about giving the dynamic datacat tree a usable static type, without changing how it behaves at runtime.
First, I added CatalogNamespace in catalog.py (line 90). Before this, every intermediate node in datacat was just a SimpleNamespace, which static analyzers treat as basically unstructured. CatalogNamespace still inherits from SimpleNamespace, so runtime behavior stays the same, but inside TYPE_CHECKING it declares that names like load, extract, data, and _ledger_endpoint are BlobEndpoint, and that arbitrary deeper attributes are more CatalogNamespace via getattr. That is what lets a chain like datacat.public.stf.nhsn_hrd_prelim.load stay typed instead of collapsing to Any.
Second, I added type-only endpoint attributes on DatasetEndpoint in catalog.py (line 117). At runtime the class was already attaching .load, .extract, and similar attributes in init, but statically those attributes did not exist on the class definition. Declaring them under TYPE_CHECKING tells editors that a dataset endpoint really does have those stage attributes and that each one is a BlobEndpoint.
Third, I changed dict_to_sn() to return CatalogNamespace instead of SimpleNamespace in catalog.py (line 744), and changed the implementation to instantiate CatalogNamespace in catalog.py (line 757). That matters because this helper is what recursively builds the whole exported namespace tree. If it returned SimpleNamespace, the typing information would still be lost immediately.
Finally, I annotated the exported datacat and reportcat objects as CatalogNamespace in catalog.py (line 800). That makes the public export itself typed at the point where it is created, so when users import datacat, the checker sees a recursive typed namespace rather than an untyped SimpleNamespace.
So the net effect is:
intermediate namespace nodes are statically typed
dataset nodes are statically typed as having .load/.extract
.load resolves to BlobEndpoint
once .load is known to be BlobEndpoint, the existing get_dataframe() overloads work as intended for "lazy" and "pl_lazy" as polars.LazyFrame
Runtime API shape is unchanged; this is purely a static typing improvement layered onto the existing dynamic construction.