refactor(to_pandas): single resource classes delegate to_pandas() to their list class - #2761
Conversation
![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 refactors the to_pandas implementation across various CogniteResource subclasses by delegating the conversion logic to their corresponding list classes, reducing duplication. This is supported by introducing a _LIST_CLASS attribute on resource classes and adding meta-tests to enforce this relationship. Feedback on the changes suggests replacing a runtime assert statement in instances.py with an explicit if check and raising a RuntimeError to ensure defensive programming and runtime safety when Python is run with optimization flags.
1046ab0 to
eb5edf5
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2761 +/- ##
==========================================
- Coverage 93.04% 93.02% -0.02%
==========================================
Files 514 514
Lines 52924 52959 +35
==========================================
+ Hits 49241 49266 +25
- Misses 3683 3693 +10
🚀 New features to boost your workflow:
|
eb5edf5 to
b2b958a
Compare
| df = super().to_pandas( | ||
| expand_metadata=False, ignore=ignore, camel_case=camel_case, convert_timestamps=convert_timestamps | ||
| assert self._LIST_CLASS is not None | ||
| df = cast("type[DataModelingInstancesList]", self._LIST_CLASS)([self]).to_pandas( |
There was a problem hiding this comment.
Why do we need to override this and not just defer to the base CogniteResource implementation? Will it not use the DataModelingInstancesList implementation if we do that?
There was a problem hiding this comment.
We do want to delegate to DataModelingInstancesList, but we cannot use the generic CogniteResource.to_pandas path directly as the base method always forwards args like expand_metadata and metadata_prefix, while DataModelingInstancesList.to_pandas uses e.g. expand_properties/remove_property_prefix.
There was a problem hiding this comment.
A lot of garbage in this repo tbh 😅 The one-base-class-fits-all pattern for example...
Just for efficiency reasons, repeated calling a function for millions of items in a list isn't exactly cheap in Python 😅 ...or, a much better reason is that a pandas dataframe concatenation from individual dataframes would be a really bad idea (just making all those individual ones) |
| df = super().to_pandas( | ||
| expand_metadata=False, ignore=ignore, camel_case=camel_case, convert_timestamps=convert_timestamps | ||
| assert self._LIST_CLASS is not None | ||
| df = cast("type[DataModelingInstancesList]", self._LIST_CLASS)([self]).to_pandas( |
|
🦄 |
haakonvt
added
risk-review-ongoing
3e92466 to
b3cee43
Compare
|
🦄 |
2 participants
Split from #2724
Removes duplicated
to_pandaslogic between resource classes and their list counterparts (e.g.Group/GroupList,Node/NodeListetc.) by having single resources delegate to_LIST_CLASS([self]).to_pandas(...)and squeeze the result, instead of maintaining a second copy of the conversion logic.Adds
_LIST_CLASS/_RESOURCEback-reference enforcement as a new test intest_meta.py: everyCogniteResourceList._RESOURCEmust point to a resource class whose_LIST_CLASSpoints right back, so future additions can't silently drift out of sync. Also addstest_standalone_to_pandas_allowlist, documenting the deliberate exceptions that don't follow this pattern.Pure refactor, no behavior change. Paves the way for the pandas v2/v3 fix.
Tip to reviewer
Almost all of the changes is part of just one commit, cd3b63f, (57 files changed, 482 insertions(+)). The PR can be reviewed commit-by-commit