Setting namespace to prevent warning #97
Conversation
WalkthroughIn several versions of the manifest parsers ( Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- dbt_artifacts_parser/parsers/manifest/manifest_v6.py (1 hunks)
- dbt_artifacts_parser/parsers/manifest/manifest_v7.py (1 hunks)
- dbt_artifacts_parser/parsers/manifest/manifest_v8.py (1 hunks)
- dbt_artifacts_parser/parsers/manifest/manifest_v9.py (1 hunks)
Files skipped from review due to trivial changes (1)
- dbt_artifacts_parser/parsers/manifest/manifest_v6.py
Additional comments not posted (3)
dbt_artifacts_parser/parsers/manifest/manifest_v8.py (1)
1084-1085: LGTM!The addition of
model_configwithprotected_namespacesset to an empty tuple effectively addresses the Pydantic warning related to the protected namespace conflict.dbt_artifacts_parser/parsers/manifest/manifest_v9.py (1)
1241-1242: LGTM!The addition of
model_configwithprotected_namespacesset to an empty tuple effectively addresses the Pydantic warning related to the protected namespace conflict.dbt_artifacts_parser/parsers/manifest/manifest_v7.py (1)
1723-1724: LGTM! Addition ofmodel_configaligns with PR objectives.The addition of
model_configwithprotected_namespacesset to an empty tuple prevents warnings related to the protected namespace "model_".
|
@yu-iskw Gentle nudge on this one. I'm building a tool on top of |
|
@pgoslatara Thank you for raising it. But, can you figure out if there is another approach to turn off the warnings, as I don't want to directly edit generated classes due to the maintainability. The discussion in #103 might be useful to understand the context behind it. For instance, I tried to introduce base classes by the artifact type in order to control model configs. But, unfortunately, I don't have much time to delve into the approach at the moment. |
|
@yu-iskw Thanks for looking into this PR! Please note that I am rather new to Pydantic so my understanding/terminology may be incorrect.
Yes! Another approach to silence the warning would be this: import warnings
with warnings.catch_warnings():
warnings.filterwarnings(
"ignore",
category=UserWarning
)
import dbt_artifacts_parserMy concern with this approach is that I have to apply it in every file where |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- dbt_artifacts_parser/parsers/manifest/manifest_v6.py (1 hunks)
- dbt_artifacts_parser/parsers/manifest/manifest_v7.py (1 hunks)
- dbt_artifacts_parser/parsers/manifest/manifest_v8.py (1 hunks)
- dbt_artifacts_parser/parsers/manifest/manifest_v9.py (1 hunks)
Additional comments not posted (4)
dbt_artifacts_parser/parsers/manifest/manifest_v8.py (1)
1085-1086: Approve addition ofmodel_configwith emptyprotected_namespaces.The addition of
model_configwithprotected_namespacesset to an empty tuple in theMetricclass is a targeted solution to the namespace conflict warning. This change aligns with the PR objectives and follows Pydantic's recommendations.However, consider adding a comment explaining this configuration to aid future maintainability.
+ # Setting protected_namespaces to an empty tuple to avoid namespace conflict warnings as per Pydantic's documentation model_config = ConfigDict(protected_namespaces=())dbt_artifacts_parser/parsers/manifest/manifest_v9.py (1)
1242-1243: Approved: Namespace conflict resolution in theMetricclass.The addition of
model_config = ConfigDict(protected_namespaces=())effectively addresses the namespace conflict warning by setting protected namespaces to an empty tuple. This change aligns with the PR's objective and follows Pydantic's recommendations for handling such conflicts.Ensure to verify that this change does not inadvertently affect other functionalities or interactions within the system that rely on the
Metricclass's configuration.dbt_artifacts_parser/parsers/manifest/manifest_v6.py (1)
1610-1610: Approved change toprotected_namespacesconfiguration.The modification to set
protected_namespacesto an empty tuple in theParsedMetricclass is a direct response to the namespace conflict warning described. This change should prevent the warning without altering the functionality of the parsing process.The code change is approved.
It's recommended to verify that this configuration change does not affect other parts of the system that might rely on the default behavior of Pydantic. Consider adding tests or checks to ensure that no other functionalities are compromised.
Verification successful
Consistent Configuration Across Manifest Versions
The
protected_namespacesconfiguration is consistently set to an empty tuple across multiple manifest versions (v6,v7,v8,v9). This indicates a deliberate strategy to handle namespace conflicts, and there is no evidence that other parts of the system rely on the default behavior of Pydantic'sprotected_namespaces. The change inmanifest_v6.pyis consistent with this approach and does not adversely affect other parts of the system.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no other parts of the system are affected by the change in protected namespaces. # Test: Search for any other references to `protected_namespaces` in the codebase. Expect: No other modifications related to this setting. rg --type python "protected_namespaces"Length of output: 70
Script:
#!/bin/bash # Corrected script to search for any references to `protected_namespaces` in the codebase. rg "protected_namespaces" --glob "*.py" -A 3Length of output: 1293
dbt_artifacts_parser/parsers/manifest/manifest_v7.py (1)
1724-1725: Review: Settingprotected_namespacesto an empty tuple inParsedMetric.The modification to set
protected_namespacesto an empty tuple is intended to resolve a namespace conflict warning as described in the PR. This change follows the recommendation from Pydantic's documentation to handle such conflicts. It's crucial to ensure that this adjustment does not inadvertently affect other parts of the system that rely on the default namespace protections provided by Pydantic.The change is approved as it directly addresses the issue described. However, it's recommended to monitor for any unintended side effects in areas of the codebase that interact with instances of
ParsedMetric.
When importing the
parse_manifestfunction, a Pydantic warning is raised. This PR follows the Pydantic docs by overwriting the default protected namespace to prevent this warning (and the unsightly message it prints to console).Replication:
The warning is also visible in previous runs of the
test.ymlworkflow, example here.