cognite-sdk-python icon indicating copy to clipboard operation
cognite-sdk-python copied to clipboard

Improve Cognite base classes

Open haakonvt opened this issue 2 years ago • 2 comments

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 avatar Feb 17 '23 15:02 haakonvt

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%) :arrow_down:
cognite/client/data_classes/data_sets.py 91.04% <ø> (+7.92%) :arrow_up:
cognite/client/data_classes/events.py 93.00% <ø> (+10.24%) :arrow_up:
cognite/client/data_classes/raw.py 88.46% <66.66%> (-9.04%) :arrow_down:
cognite/client/exceptions.py 88.88% <66.66%> (+0.22%) :arrow_up:
...ognite/client/data_classes/transformations/jobs.py 89.58% <71.42%> (-1.73%) :arrow_down:
cognite/client/data_classes/_base.py 86.09% <77.56%> (-8.98%) :arrow_down:
... and 24 more

codecov[bot] avatar Apr 26 '23 15:04 codecov[bot]

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

haakonvt avatar Apr 28 '23 15:04 haakonvt