refactor!: [V8] Split container classes into read/write versions (DM-3215) #2382

audunska · 2025-10-27T09:27:56Z

The Container class already had a ContainerApply write version, but the member classes didn't. Since we added state fields, the read and write versions now differ, so split them.

Also converted them to dataclasses.

@haakonvt feel free to ignore this PR until you've gotten the alpha version out :)

gemini-code-assist · 2025-10-27T09:28:00Z

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

audunska · 2025-10-27T17:31:36Z

/gemini review

gemini-code-assist

Code Review

This pull request is a significant and well-executed refactoring of the container-related data classes. The introduction of dataclasses and the separation of read and write models (Container vs. ContainerApply) greatly improve code clarity, type safety, and maintainability, aligning well with the repository's style guide. The use of ...Core base classes to abstract common functionality is an excellent pattern.

However, I've identified a critical issue in the new implementation. The state field in the read versions of Constraint and Index classes has been changed from optional to required. This could lead to KeyError exceptions if the API omits this field in its responses, which the previous implementation handled gracefully. I've provided detailed comments and suggestions to address this potential regression.

cognite/client/data_classes/data_modeling/containers.py

…acker

…Handler

- fix tests/tests_unit/test_api/test_assets.py - fix tests/tests_unit/test_api/test_data_modeling/test_containers.py - fix tests/tests_unit/test_api/test_data_sets.py - fix tests/tests_unit/test_api/test_datapoints.py - fix tests/tests_unit/test_api/test_documents.py - fix tests/tests_unit/test_api/test_entity_matching.py - fix tests/tests_unit/test_api/test_events.py - fix tests/tests_unit/test_api/test_extractionpipelines.py - fix tests/tests_unit/test_api/test_files.py (puh!) - fix tests/tests_unit/test_api/test_functions.py (mega puuh) - fix tests/tests_unit/test_api/test_iam.py - fix tests/tests_unit/test_api/test_labels.py - fix tests/tests_unit/test_api/test_raw.py - fix tests/tests_unit/test_api/test_relationships.py - fix tests/tests_unit/test_api/test_sequences.py (puh again) - fix tests/tests_unit/test_api/test_synthetic_time_series.py - fix tests/tests_unit/test_api/test_time_series.py - fix tests/tests_unit/test_api/test_vision_extract.py - fix tests/tests_unit/test_api_client.py ( 👀 ) - fix tests/tests_unit/test_cognite_client.py - fix tests/tests_unit/test_data_classes/test_capabilities.py - fix tests/tests_unit/test_http_client.py - fix tests/tests_unit/test_meta.py

also use dataclasses more

haakonvt

Thanks for having the patience to get all these classes split into proper read and write versions 😄 The removal of object.__setattr__ also brings joy to my heart!

A few questions:

haakonvt · 2025-10-28T20:20:28Z

cognite/client/data_classes/data_modeling/containers.py

        return self.as_id().as_property_ref(property)


+@dataclass(frozen=True)


I'm unsure about frozen on write classes.

I wanted to remove frozen, but you're not allowed to inherit frozen classes from non-frozen classes and vice versa. So they all need to be frozen or not.

Huh, I see. That makes me a bit sad 🥲 Thoughts @erlendvollset ?

haakonvt · 2025-10-28T20:21:57Z

cognite/client/data_classes/data_modeling/containers.py

+            property_loader=ContainerPropertyApply._load,
+            constraint_loader=ConstraintApply._load,
+            index_loader=IndexApply._load,


We seem to be always calling ._load on the passed callables - then I think I would prefer to pass the classes instead.

The problem was with the Constraint and Index classes. If we switch the argument to constraint_class: type[ConstraintCore] and pass constraint_class=Constraint or constraint_class=ConstraintApply, mypy will complain with

cognite/client/data_classes/data_modeling/containers.py:127: error: Only concrete class can be given where "type[ConstraintCore]" is expected [type-abstract]

So I switched to using loader functions for these arguments.

We can still pass classes for the property. Should we?

Oh how I love mypy 🙃 Does it work with a TypeVar using bound?

Anyway, feel free to just leave it as is 👍

haakonvt · 2025-10-28T20:22:59Z

cognite/client/data_classes/data_modeling/containers.py

+    is_global: bool = False
+    last_updated_time: int = 0
+    created_time: int = 0
+    used_for: Literal["node", "edge", "all"] = "all"


Why do these have default values?

Good point. They shouldn't.

@haakonvt I removed all defaults from the read classes and made them kw_only. We don't care about these being convenient to construct.

The write classes can have defaults, but I wonder if we might want to make them kw_only too?

Nice, agree on the read classes 👍

I think we shouldn't force the user into a specific way to instantiate the write classes.

haakonvt · 2025-10-28T20:28:59Z

cognite/client/data_classes/data_modeling/containers.py

+            nullable=resource.get("nullable", True),
+            auto_increment=resource.get("autoIncrement", False),


Could you elaborate a bit on the change away from the previous behaviour? (Wrt # If nothing is specified, we will pass through null values)

Maybe not a very conscious decision. I just found it cleaner to use sensible defaults where appropriate. But maybe we should keep them as optional and retain the None pass-through. What do you think?

Having thought some more about it, I think your changes make sense.

Would like to know the reason for the previous behaviour choice though

and make them `kw_only`

haakonvt · 2025-11-12T09:25:41Z

Merged into #2387 , closing

audunska requested review from a team as code owners October 27, 2025 09:27

audunska changed the base branch from master to v8-change-requests-to-httpx October 27, 2025 09:28

audunska changed the title ~~refactor!(v8): Split container classes into read/write versions (DM-3215)~~ refactor! [V8]: Split container classes into read/write versions (DM-3215) Oct 27, 2025

audunska changed the title ~~refactor! [V8]: Split container classes into read/write versions (DM-3215)~~ refactor!: [V8] Split container classes into read/write versions (DM-3215) Oct 27, 2025

audunska force-pushed the v8-split-container-read-write branch from 1c7537d to 1a7da07 Compare October 27, 2025 09:30

gemini-code-assist bot reviewed Oct 27, 2025

View reviewed changes

cognite/client/data_classes/data_modeling/containers.py Show resolved Hide resolved

cognite/client/data_classes/data_modeling/containers.py Show resolved Hide resolved

haakonvt and others added 21 commits October 27, 2025 20:10

add httpx and poetry update

18ee841

add: unpack_items function

3d0e5ff

refactor: move url methods to utils/_url.py. add VALID_URL_PATTERN

43c885d

remove playground

84838f6

refactor: split out basic api methods in BasicAPIClient

f51985a

update HTTPClient(WithRetry), HTTPClient(WithRetry)Config and RetryTr…

8d60db1

…acker

update GlobalConfig and ClientConfig

7ef9079

update APIClient/BasicAPIClient GET/POST etc, introduce FailedRequest…

99bcea4

…Handler

update exceptions. always use specialized not-found/duplicated err

1bbb7bc

add drop_none_values to fix httpx usage of url params that are null

bbcca7a

update APIs using _do_request or stream

a4543b0

dependency: swap responses with httpx_mock

708399c

remove dependency pyodide-http

12c2596

clean up pyodide patch script

16cef64

add authlib, remove requests-oauthlib

dd3698d

use httpx-backed authlib for client credz auth flow

678e54a

update the wording/warning on 'HTTP Request logging'

77b3802

update .gemini/styleguide.md with httpx.Response

86a4311

add EventLoopThreadExecutor + global_config.event_loop

921bec1

Fix http client usage in organization API

cac29b4

Update InstancesAPI.subscribe doc example to use -Sync classes

9f8f2c5

haakonvt force-pushed the v8-change-requests-to-httpx branch 2 times, most recently from e92c87f to 6a22e7e Compare October 28, 2025 11:57

haakonvt added 7 commits October 28, 2025 20:53

never ending TMP

0fe1aa8

make BasicAsyncAPIClient picklable

155d473

turn on pickle-client-test and add one for AsyncCogniteClient

edbae4c

Update MIGRATION_GUIDE.md for v7 -> v8

67737d3

BUMP VERSION to 8.0.0a1

2142f74

WIP Cursor wrote migration guide

8923004

TMP

016505a

haakonvt force-pushed the v8-change-requests-to-httpx branch from 3f22dcd to 016505a Compare October 28, 2025 20:13

audunska added 3 commits October 28, 2025 21:14

Split the container classes into read/write

a3699a0

also use dataclasses more

Update docstrings in api

ee1ff0e

Update tests

0b8f877

haakonvt force-pushed the v8-split-container-read-write branch from 1a7da07 to 0b8f877 Compare October 28, 2025 20:14

haakonvt reviewed Oct 28, 2025

View reviewed changes

Remove all defaults from read classes

eadb735

and make them `kw_only`

haakonvt force-pushed the v8-change-requests-to-httpx branch 10 times, most recently from c605eee to 1ba5b84 Compare November 10, 2025 13:27

haakonvt closed this Nov 12, 2025

haakonvt deleted the v8-split-container-read-write branch November 12, 2025 09:25

		return self.as_id().as_property_ref(property)


		@dataclass(frozen=True)

		nullable=resource.get("nullable", True),
		auto_increment=resource.get("autoIncrement", False),

refactor!: [V8] Split container classes into read/write versions (DM-3215) #2382

refactor!: [V8] Split container classes into read/write versions (DM-3215) #2382

Uh oh!

Conversation

audunska commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot commented Oct 27, 2025

Uh oh!

audunska commented Oct 27, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

haakonvt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haakonvt commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

audunska commented Oct 27, 2025 •

edited

Loading