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

Improve Cognite base classes #1151

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Conversation

haakonvt
Copy link
Contributor

@haakonvt haakonvt commented Feb 17, 2023

Description

New faster implementation for _cognite_client private attr:

  1. Removes the custom __new__ implementation for a 2X object instantiation speed up.
  2. With __new__ removed, subclasses of CogniteResource no longer have an implicit __init__ that just swallows all args and all kwargs.
  3. Removes custom __getattribute__ implementation (extra hook before regular attribute lookup). This change adds a speed up to all attribute accesses.
  4. Adds _cognite_client as a simple property (see class _WithClientMixin) with a setter method that guarantees a valid value (Optional[CogniteClient]) and a getter that raises CogniteMissingClientError in all cases except where the attribute is an instance of CogniteClient.

New faster _load implementation

  1. Removes the old if hasattr -> setattr implementation that called to_snake_case O(N*M) times (N dicts, M elements per dict).
  2. Changed to using the newly added fast_dict_load function, which uses get_accepted_params that caches the accepted parameters for each class it receives, returning a dict that maps camelCase to snake_case for very efficient API response translation into our data classes.

New hierarchy to avoid code duplication

  1. Introduces a CogniteBase class with __str__, __repr__, __eq__ and dump methods.
  2. Introduces a CogniteBaseList class which...
    1. Adds all missing dunder- and list methods necessary for us to override from UserList.
    2. This means we no longer leak memory because of the internal dict lookup on id and external_id that keeps a reference to elements after deletion from the list.
    3. This also adds duplicate checks that raise on non-preserving actions.
  3. Adds the missing CogniteResponseList (note not Resource)

Other changes / optimizations:

  1. _load methods now force cognite_client to be passed as these are "only" used by the SDK. This allows better visibility into all the different classes that do not use it (and could be moved to a different parent class without the client in the future).
  2. With these changes, mypy now better understands the code base. This has resulted in 100+ issues that have been changed or fixed.
  3. The error message on CogniteMissingClientError has been improved.
  4. New tests / check for the various data classes: Verification of base subclasses has been moved to tests (TestVerifyAllCogniteSubclasses).
  5. CogniteResponse no longer accepts cognite_client.
  6. CogniteFilter and subclasses had many unused _load functions that have been removed (these are never instantiated by the SDK, as opposed to most other resource classes).
  7. Removes EXCLUDE_VALUE in favor of a simple None-check (was list search with one element). YAGNI 😄

@haakonvt haakonvt force-pushed the improve-base-cog-classes branch 2 times, most recently from 2557697 to 9d22024 Compare April 5, 2023 10:04
@haakonvt haakonvt force-pushed the improve-base-cog-classes branch 4 times, most recently from e6958a7 to ba4a3f7 Compare April 26, 2023 11:56
@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #1151 (f4807c6) into master (05e2b3f) will increase coverage by 0.25%.
The diff coverage is 82.13%.

@@            Coverage Diff             @@
##           master    #1151      +/-   ##
==========================================
+ Coverage   90.63%   90.89%   +0.25%     
==========================================
  Files          82       82              
  Lines        9899     9906       +7     
==========================================
+ Hits         8972     9004      +32     
+ Misses        927      902      -25     
Impacted Files Coverage Δ
cognite/client/_api/datapoints.py 96.30% <ø> (ø)
cognite/client/_api/sequences.py 95.75% <ø> (ø)
cognite/client/_api/three_d.py 97.61% <ø> (ø)
cognite/client/data_classes/annotations.py 96.38% <ø> (-0.05%) ⬇️
cognite/client/data_classes/data_sets.py 91.04% <ø> (+7.92%) ⬆️
cognite/client/data_classes/events.py 93.00% <ø> (+10.24%) ⬆️
cognite/client/data_classes/raw.py 88.46% <66.66%> (-9.04%) ⬇️
cognite/client/exceptions.py 88.88% <66.66%> (+0.22%) ⬆️
...ognite/client/data_classes/transformations/jobs.py 89.58% <71.42%> (-1.73%) ⬇️
cognite/client/data_classes/_base.py 86.09% <77.56%> (-8.98%) ⬇️
... and 24 more

@haakonvt haakonvt force-pushed the improve-base-cog-classes branch 3 times, most recently from 4bdaba2 to c68fc9a Compare April 27, 2023 13:25
@haakonvt haakonvt changed the title Draft: Improve base cognite classes Improve Cognite base classes Apr 28, 2023
@haakonvt haakonvt marked this pull request as ready for review April 28, 2023 15:03
@haakonvt haakonvt requested review from a team as code owners April 28, 2023 15:03
@haakonvt
Copy link
Contributor Author

haakonvt commented Apr 28, 2023

Interested in feedback on how to make the class hierarchy better!

  • Type hints still feel lacking in many places, should we add typing_extensions ^4 to get Self for instance?

Note #1: I'll add tests once design is decided on 👌

Note #2: I can try to split this PR into several

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant