-
Notifications
You must be signed in to change notification settings - Fork 1
Upgrade to Pydantic v2 and bump version to 0.1.0 #76
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
Conversation
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.
Important
Looks good to me! 👍
Reviewed everything up to deec9e9 in 2 minutes and 28 seconds. Click for details.
- Reviewed
610lines of code in15files - Skipped
1files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:1
- Draft comment:
Overall, the changes for the Pydantic v2 upgrade look consistent. The shift to usingConfigDictfor model configuration and updating validation calls (e.g. usingmodel_validate()) is correctly applied. However, please double‐check that mutable defaults (such as empty lists or dicts provided via=[]or{}) are handled appropriately in V2. In many cases, Pydantic v2 recommends using a default_factory (for example,Field(default_factory=list)) to prevent unintended shared mutable state. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:116
- Draft comment:
The usage of Annotated with StringConstraints for fields like user_id is clear and follows V2 best practices. Confirm that the regex pattern captures the identifiers as expected. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:125
- Draft comment:
Check that the replacement of calls (e.g. using .model_validate in default_factory lambdas) is working as expected and doesn’t introduce performance issues. It might be worth adding benchmarks or tests if these functions are called frequently. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v12.py:2389
- Draft comment:
Typo: There is an extra closing bracket in the type annotation for 'severity'. It should likely beOptional[Annotated[str, StringConstraints(pattern=r"^([Ww][Aa][Rr][Nn]|[Ee][Rr][Rr][Oo][Rr])$")] = "ERROR"(with only one closing bracket after StringConstraints). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment appears to be incorrect. The type annotation has the correct number of closing brackets: 1. One bracket closes StringConstraints(...) 2. One bracket closes Annotated[str, ...] This is the proper syntax for using Annotated with StringConstraints. Could I be misunderstanding how Annotated types should be formatted in Python? Should I double check the Python typing documentation? No, I'm confident about this. The Python typing documentation confirms that Annotated[T, X] takes two type parameters, and StringConstraints is a valid constraint annotation. The brackets are correctly paired. The comment should be deleted because it is incorrect - the type annotation has the proper number of closing brackets.
5. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v5.py:29
- Draft comment:
There seems to be an extra closing bracket in the type annotation foruser_id. It should likely beOptional[Annotated[str, StringConstraints(pattern=r"[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}")]]with the correct matching brackets (i.e., one bracket to close the Annotated type and one for Optional). Please verify the bracket balance. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The type annotation isOptional[Annotated[str, StringConstraints(...)]]which has correct bracket nesting - one closing bracket for the inner Annotated and one for the outer Optional. The comment is incorrect in suggesting there is an extra bracket. Could I be misunderstanding the bracket nesting rules for Python type hints? Should I double check the Python typing documentation? No, I am confident about Python type hint syntax. The nesting of Optional[Annotated[...]] is standard and correct here. The comment is simply wrong. Delete the comment because it incorrectly claims there is a bracket balance issue when the code is actually correct.
6. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v7.py:29
- Draft comment:
Typo: There appears to be an extra closing bracket in the type annotation. Instead of '...StringConstraints(...)]]', it should be '...StringConstraints(...)]'. Please fix the bracket count. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is incorrect. The double brackets are not a typo - they are required because: 1. The outer bracket is for Optional[] 2. The inner bracket is for Annotated[] This is standard Python type annotation syntax when combining Optional with Annotated types. Could there be a different version of Python or pydantic where this syntax is invalid? Could the comment author be seeing something I'm missing? No - this is standard typing syntax that works across Python versions. The code is using typing_extensions.Annotated which specifically requires this bracket structure. The comment should be deleted because it incorrectly identifies valid Python type annotation syntax as a typo.
7. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v7.py:108
- Draft comment:
There's a typographical issue with the 'severity' field: an extra closing bracket is present in the type annotation. It should beOptional[Annotated[str, StringConstraints(pattern=r"^([Ww][Aa][Rr][Nn]|[Ee][Rr][Rr][Oo][Rr])$")]]instead of having an additional bracket at the end. - Reason this comment was not posted:
Comment looked like it was already resolved.
8. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v8.py:29
- Draft comment:
Typographical error: There's an extra closing bracket in the type annotation foruser_id. The annotation readsOptional[Annotated[str, StringConstraints(...)]])with]]which appears to be a mistake. It likely should be corrected to have just one closing bracket. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is incorrect. The type annotation uses nested type wrappers (Optional and Annotated) which require multiple closing brackets. The pattern is: Optional[Annotated[str, Constraints()]] which needs exactly two closing brackets to match the opening ones. This is valid Python type annotation syntax. Could there be some other context or style guide that prefers a different bracket structure? Could the commenter be seeing something in the rendered display that makes it look wrong? No - this is standard Python type annotation syntax for nested type wrappers, and the brackets are required for valid Python. The comment is objectively incorrect about the syntax. The comment should be deleted because it incorrectly claims there is a syntax error when the bracket structure is actually correct and necessary.
Workflow ID: wflow_xyWVtGbznaIiSc3z
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
deec9e9 to
0a7040e
Compare
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.
Important
Looks good to me! 👍
Reviewed 0a7040e in 2 minutes and 54 seconds. Click for details.
- Reviewed
623lines of code in16files - Skipped
1files when reviewing. - Skipped posting
9draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:20
- Draft comment:
Good use of pydantic v2’s ConfigDict for model config settings. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:57
- Draft comment:
Several fields (e.g. lists and dicts) use mutable defaults (e.g. [] and {}). Consider using default_factory to avoid shared mutable state. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:96
- Draft comment:
Default mutable values in TestConfig and similar classes should use default_factory (e.g. for tags and meta) to prevent unintended sharing. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:125
- Draft comment:
Consider reviewing auto-generated schema defaults. Using defaults for list and dict fields directly may lead to mutable default pitfalls in pydantic v2. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:243
- Draft comment:
Review the use of Optional[Annotated[str, StringConstraints(...)]]. The usage seems correct for validating user_id fields. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:115
- Draft comment:
Overall, the migration to pydantic v2 using model_config and ConfigDict is consistent. Consider adding docstrings or comments to clarify model purposes if not auto-documented. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
7. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v12.py:3019
- Draft comment:
Typo: There is an extra closing bracket in the type annotation ofseverity. Instead ofOptional[Annotated[str, StringConstraints(...)]])it should only have one closing bracket, i.e.,Optional[Annotated[str, StringConstraints(...)]]. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment appears to be incorrect. It claims there is an extra closing bracket, but comparing the original and suggested code shows they are identical. The double closing brackets are actually correct - one for closing the Annotated[] and one for closing the Optional[]. This is a false positive from the automated tool. Could I be misreading the syntax? Maybe there's some subtle Python type annotation rule I'm not aware of? No, I'm confident in this case. The double brackets are required because we have nested type constructors - Optional[] wrapping Annotated[]. The suggestion in the comment would make the type annotation invalid. The comment should be deleted because it is incorrect - there is no extra closing bracket. The existing type annotation is syntactically correct with the proper number of closing brackets for the nested type constructors.
8. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v6.py:29
- Draft comment:
There is a typographical error on this line: an extra ']' is present after the StringConstraints annotation. Please remove the extra bracket to correctly close the annotation as 'Annotated[str, StringConstraints(...)]'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment claims there is an extra bracket, but looking at the code carefully, the brackets are correctly balanced. This is a valid Python type annotation using nested generic types. The comment author may have been confused by the nested nature of the type annotations. Could I be misunderstanding how Python type annotations work with nested generics? Should I double check the Python typing documentation? No, I'm confident in this case. The syntax follows the standard pattern for nested type annotations in Python, and the code is clearly generated by a tool (datamodel-codegen) which is unlikely to generate invalid syntax. The comment should be deleted as it incorrectly claims there is a syntax error when the code is actually correct.
9. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:141
- Draft comment:
Typo: There is an extra closing bracket in the type annotation for 'severity'. It should likely be Optional[Annotated[str, StringConstraints(pattern=r"^([Ww][Aa][Rr][Nn]|[Ee][Rr][Rr][Oo][Rr])$")]] (with one closing bracket after StringConstraints) rather than having an extra bracket. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_h7D47CPU5W0AbEwx
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Pull Request Overview
This PR upgrades the codebase from Pydantic v1 to v2, which includes breaking changes requiring version bump to 0.1.0. The migration involves updating model configuration syntax, fixing field requirements, and modernizing deprecated method calls.
Key changes:
- Updated Pydantic model configuration from
class Config:tomodel_config = ConfigDict() - Added default values (
= None) to Optional fields without defaults for Pydantic v2 compatibility - Replaced deprecated
.json()method calls with.model_dump_json()
Reviewed Changes
Copilot reviewed 4 out of 17 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/datapilot/core/platforms/dbt/schemas/manifest.py | Added default values to Optional fields without defaults |
| src/datapilot/core/platforms/dbt/schemas/catalog.py | Updated model configuration syntax and imports |
| src/datapilot/core/platforms/dbt/executor.py | Replaced deprecated .json() with .model_dump_json() |
| setup.py | Bumped version from 0.0.26 to 0.1.0 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
This package was already using pydantic v2 ("pydantic >=2.0,<3.0",) |
Got it, I see yu-iskw/dbt-artifacts-parser#171 |
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.
Important
Looks good to me! 👍
Reviewed 5f8f8d5 in 1 minute and 5 seconds. Click for details.
- Reviewed
441lines of code in12files - Skipped
1files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:1
- Draft comment:
The migration to Pydantic v2 is correctly implemented using ConfigDict and Field; the overall syntax update appears complete. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:29
- Draft comment:
Usage of constr with a regex for the 'user_id' field is correct and follows best practices. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that the usage is correct and follows best practices, which is not necessary for the PR review process.
3. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:100
- Draft comment:
Many models use default_factory lambdas (e.g. calling Model.model_validate({...})); while this follows the new Pydantic v2 pattern, if these defaults are static, consider extracting them to module-level constants to reduce overhead. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:1250
- Draft comment:
The 'nodes' field in the ManifestV9 model unions a very large set of node types; ensure that runtime type validation performance is acceptable. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:95
- Draft comment:
Field naming like 'schema_' with an alias of 'schema' is used consistently to avoid reserved word conflicts. This is a good practice. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:560
- Draft comment:
The file header already indicates it is auto-generated. Ensure that any manual changes are applied to the code generation template to avoid being overwritten. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_v9.py:700
- Draft comment:
Repeating default_factory lambdas for nested models (e.g. Docs, DependsOn) may be optimized by reusing constant defaults if they are static. This is a minor performance/maintenance consideration. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_qcOS9gsDJD4f20ZS
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This commit completes the migration from Pydantic v1 to v2, with the following changes: - Updated Pydantic model configuration syntax: - Changed `class Config:` to `model_config = ConfigDict()` in catalog.py - Updated imports to use `from pydantic import ConfigDict` - Fixed Pydantic v2 field requirements: - Added default values (= None) to all Optional fields without defaults in manifest.py - This is required in Pydantic v2 for proper model validation - Updated deprecated method calls: - Changed `.json()` to `.model_dump_json()` in executor.py - These methods serialize Pydantic models to JSON strings - Added .venv/ to .gitignore - Bumped version from 0.0.26 to 0.1.0 to reflect breaking changes Note: Vendored manifest_v*.py files in src/vendor/dbt_artifacts_parser/ remain unchanged as they are historical archives. Testing: - Ran full test suite using .venv/bin/pytest - All 64 tests passed successfully - Verified Pydantic 2.12.0 compatibility BREAKING CHANGE: This upgrade requires downstream users to also upgrade to Pydantic v2. setup.py now specifies pydantic>=2.0,<3.0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
5f8f8d5 to
d4844c0
Compare
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.
Important
Looks good to me! 👍
Reviewed d4844c0 in 1 minute and 42 seconds. Click for details.
- Reviewed
182lines of code in4files - Skipped
1files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. setup.py:16
- Draft comment:
Version bump and dependency update for Pydantic v2 look correct. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide any actionable feedback or suggestions. It simply states that the version bump and dependency update look correct, which is not allowed according to the rules.
2. src/datapilot/core/platforms/dbt/executor.py:112
- Draft comment:
Updated to use model_dump_json(); ensure its JSON output matches expected legacy structure. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. src/datapilot/core/platforms/dbt/schemas/catalog.py:63
- Draft comment:
Migration to Pydantic v2 using 'model_config = ConfigDict(extra="allow")' is correctly applied. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, stating that a migration to Pydantic v2 is correctly applied. It does not provide any actionable feedback or suggestions for improvement.
4. src/datapilot/core/platforms/dbt/schemas/manifest.py:147
- Draft comment:
Redundant 'Optional[Optional[str]]' found; simplify to 'Optional[str]' for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
5. src/datapilot/core/platforms/dbt/schemas/manifest.py:60
- Draft comment:
Avoid mutable default ({} for 'meta'); consider using a default_factory to prevent shared state. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. src/datapilot/core/platforms/dbt/schemas/manifest.py:63
- Draft comment:
Avoid using mutable default for 'tags'; use a default_factory (e.g., list) instead of [] to prevent unintended sharing. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_0H95V0YrKKQUCnfF
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Upgrade to Pydantic v2 and bump version to 0.1.0
This commit completes the migration from Pydantic v1 to v2, with the following changes:
Updated Pydantic model configuration syntax:
class Config:tomodel_config = ConfigDict()in catalog.pyfrom pydantic import ConfigDictFixed Pydantic v2 field requirements:
Updated deprecated method calls:
.json()to.model_dump_json()in executor.pyAdded .venv/ to .gitignore
Bumped version from 0.0.26 to 0.1.0 to reflect breaking changes
Note: Vendored manifest_v*.py files in src/vendor/dbt_artifacts_parser/ remain
unchanged as they are historical archives.
Testing:
BREAKING CHANGE: This upgrade requires downstream users to also upgrade to Pydantic v2.
setup.py now specifies pydantic>=2.0,<3.0.
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]
Important
Upgrade to Pydantic v2, updating model configurations, method calls, and vendor dependencies, with a version bump to 0.1.0.
catalog.pyandmanifest.py.class Config:tomodel_config = ConfigDict()..json()to.model_dump_json()inexecutor.py.Optionalfields without defaults inmanifest.py.constr()withAnnotated[str, StringConstraints()]in vendor dependencies.from typing_extensions import Annotatedimports..venv/to.gitignore.setup.py.BREAKING CHANGE: Requires downstream users to upgrade to Pydantic v2.
This description was created by
for d4844c0. You can customize this summary. It will automatically update as commits are pushed.