-
Notifications
You must be signed in to change notification settings - Fork 35
release 8.0.0a1 to PyPI (DM-3062)
#2387
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
base: pysdk-release-v8
Are you sure you want to change the base?
Conversation
Using Gemini Code AssistThe 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
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 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. |
8f8308e to
66518da
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a major version bump to 8.0.0a0, with the primary change being the introduction of full asynchronous support using httpx. This is a significant and well-executed refactoring that makes almost all API methods async. The SDK is now async-first, with a synchronous CogniteClient wrapping the new AsyncCogniteClient for backward compatibility. In addition to the async refactoring, this PR removes several deprecated methods and parameters, such as the generic aggregate and filter methods, and the partitions parameter in most __call__ methods. It also improves some APIs, like insert_dataframe which now infers identifier types. All these breaking changes are clearly documented in the updated MIGRATION_GUIDE.md. The changes are extensive but appear to be correct, consistent, and of high quality. I have not found any issues of high or critical severity.
Aasnaess
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This comment was marked as outdated.
This comment was marked as outdated.
| yield dps_lst[0] if is_single else dps_lst | ||
| elif is_single: | ||
| yield from chunk_fn(dps_lst[0]) # type: ignore [misc] | ||
| for chunk in chunk_fn(dps_lst[0]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not yield from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inside an async generator function
f05ee51 to
f0d63ae
Compare
which is used by Cognite IDP
f0d63ae to
c605eee
Compare
c605eee to
1ba5b84
Compare
| - Extending a `Datapoints` instance is no longer supported. | ||
| - **ClientConfig**: | ||
| - `max_workers` has now permanently moved to `global_config` after the deprecation period | ||
| - `timeout`: default has been increased from 30 sec to 60 sec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for this?
| def get(self, url: str, params: dict[str, Any] | None = None, headers: dict[str, Any] | None = None) -> Response: | ||
| async def get( | ||
| self, url: str, params: dict[str, Any] | None = None, headers: dict[str, Any] | None = None | ||
| ) -> httpx.Response: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should not return httpx objects, that's an abstraction leak that I'd like to get rid of. We should wrap them in our own Response object instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for the sync client
| self.disable_ssl: bool = False | ||
| self.proxies: dict[str, str] | None = {} | ||
| self.proxy: str | httpx.Proxy | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, should not deal with httpx objects directly. Wrap it in our own instead.
| sort=prep_sort, | ||
| ) | ||
|
|
||
| def __iter__(self) -> Iterator[TimeSeries]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have the __iter__ methods been removed? - not mentioned in migration guide.
|
|
||
| - id: sync-client-codegen | ||
| name: run sync codegen on changed files | ||
| entry: poetry run python scripts/sync_client_codegen/main.py run --files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails if the sync client is not up to date right?
Note to reviewer: