-
Notifications
You must be signed in to change notification settings - Fork 436
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
Prevent special whitespaces in the names of entities #2665
Prevent special whitespaces in the names of entities #2665
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent updates across the ZenML codebase primarily focus on enhancing name validation for various entities. A new constant and class ( Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
@coderabbitai 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.
Just nits. Otherwise looks good to me. I look to @bcdurak for the Pydantic perspective, however.
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.
Actionable comments posted: 2
Out of diff range and nitpick comments (31)
tests/integration/functional/zen_stores/test_zen_store.py (31)
Line range hint
1-1
: Add a module-level docstring to describe the purpose and scope of this test module.
Line range hint
148-148
: Remove the extra blank line after the function docstring to adhere to PEP 257.
Line range hint
383-383
: Add a docstring to theTestAdminUser
class to explain its purpose and functionality.
Line range hint
671-674
: Ensure there is a blank line between the summary line and the description in the docstring for better readability.
Line range hint
712-715
: Ensure there is a blank line between the summary line and the description in the docstring for better readability.
Line range hint
3029-3029
: The first line of the docstring should end with a period, question mark, or exclamation point.
Line range hint
3291-3291
: Remove the extra blank line after the function docstring to adhere to PEP 257.
Line range hint
3723-3723
: Remove the extra blank line after the function docstring to adhere to PEP 257.
Line range hint
3774-3774
: Remove the extra blank line after the function docstring to adhere to PEP 257.
Line range hint
3802-3802
: Remove the extra blank line after the function docstring to adhere to PEP 257.
Line range hint
3843-3843
: Remove the extra blank line after the function docstring to adhere to PEP 257.
Line range hint
4043-4043
: Add a docstring to theTestModel
class to explain its purpose and functionality.
Line range hint
4646-4646
: Add a docstring to theTestTag
class to explain its purpose and functionality.
Line range hint
4647-4647
: Add a docstring to thetest_create_pass
method to explain its purpose and functionality.
Line range hint
4663-4663
: Add a docstring to thetest_create_bad_input
method to explain its purpose and functionality.
Line range hint
4719-4719
: Add a docstring to thetest_create_duplicate
method to explain its purpose and functionality.
Line range hint
4753-4753
: Add a docstring to thetest_get_tag_found
method to explain its purpose and functionality.
Line range hint
4779-4779
: Add a docstring to thetest_get_tag_not_found
method to explain its purpose and functionality.
Line range hint
4805-4805
: Add a docstring to thetest_list_tags
method to explain its purpose and functionality.
Line range hint
4814-4814
: Add a docstring to thetest_update_tag
method to explain its purpose and functionality.
Line range hint
4824-4824
: Add a docstring to thetest_create_tag_resource_pass
method to explain its purpose and functionality.
Line range hint
4917-4917
: Add a docstring to theTestTagResource
class to explain its purpose and functionality.
Line range hint
4918-4918
: Add a docstring to thetest_create_tag_resource_pass
method to explain its purpose and functionality.
Line range hint
4961-4961
: Add a docstring to thetest_create_tag_resource_fails_on_duplicate
method to explain its purpose and functionality.
Line range hint
4988-4988
: Add a docstring to thetest_delete_tag_resource_pass
method to explain its purpose and functionality.
Line range hint
4996-4996
: Add a docstring to thetest_delete_tag_resource_mismatch
method to explain its purpose and functionality.
Line range hint
5006-5006
: Add a docstring to thetest_cascade_deletion
method to explain its purpose and functionality.
Line range hint
5063-5063
: Add a docstring to theTestRunMetadata
class to explain its purpose and functionality.
Line range hint
5142-5142
: Add a docstring to theTestModelVersionArtifactLinks
class to explain its purpose and functionality.
Line range hint
5281-5281
: Add a docstring to theTestModelVersionPipelineRunLinks
class to explain its purpose and functionality.
Line range hint
5287-5287
: Add a docstring to thetest_link_create_pass
method to explain its purpose and functionality.
…s://github.com/zenml-io/zenml into bugfix/OSSK-531-mask-names-of-stack-components
I made it in a bit more functional way, so no impact on Pydantic. |
@coderabbitai 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.
Actionable comments posted: 7
Out of diff range and nitpick comments (30)
tests/integration/functional/test_client.py (30)
Line range hint
1-1
: Add a module-level docstring to describe the purpose and scope of this test module.
Line range hint
557-557
: The docstring should end with a period.- """Test creating run metadata linked only to a pipeline run.""" + """Test creating run metadata linked only to a pipeline run. """
Line range hint
635-635
: The docstring should end with a period.- """Test creating metadata linked to a pipeline run and a stack component""" + """Test creating metadata linked to a pipeline run and a stack component. """
Line range hint
846-846
: Remove the blank line after the function docstring.- """Test that artifact pruning works.""" - + """Test that artifact pruning works."""
Line range hint
991-991
: Add a docstring to theTestArtifact
class to describe its purpose.
Line range hint
1179-1181
: Add a blank line between the summary line and the description in the docstring.- """Test that model version name is mutable.""" + """Test that model version name is mutable. +
Line range hint
1195-1195
: Add a docstring to theTestModelVersion
class to describe its purpose.
Line range hint
1309-1309
: Add a docstring to theTestModelVersion
class to describe its purpose.
Line range hint
1314-1314
: Add a docstring to thetest_get_model_version_by_name_found
method.
Line range hint
1328-1328
: Add a docstring to thetest_get_model_version_by_id_found
method.
Line range hint
1341-1341
: Add a docstring to thetest_get_model_version_by_index_found
method.
Line range hint
1345-1345
: Add a docstring to thetest_get_model_version_by_stage_found
method.
Line range hint
1374-1374
: Add a docstring to thetest_get_model_version_by_stage_not_found
method.
Line range hint
1378-1378
: Add a docstring to thetest_get_model_version_not_found
method.
Line range hint
1384-1384
: Add a docstring to thetest_create_model_version_pass
method.
Line range hint
1388-1388
: Add a docstring to thetest_create_model_version_duplicate_fails
method.
Line range hint
1481-1481
: Add a docstring to theTestModelVersion
class to describe its purpose.
Line range hint
1488-1488
: Add a docstring to thetest_get_model_version_by_name_found
method.
Line range hint
1497-1497
: Add a docstring to thetest_get_model_version_by_id_found
method.
Line range hint
1509-1509
: Add a docstring to thetest_get_model_version_by_index_found
method.
Line range hint
1523-1523
: Add a docstring to thetest_get_model_version_by_stage_found
method.
Line range hint
1533-1533
: Add a docstring to thetest_get_model_version_by_stage_not_found
method.
Line range hint
1552-1552
: Add a docstring to thetest_get_model_version_not_found
method.
Line range hint
1560-1560
: Add a docstring to thetest_create_model_version_pass
method.
Line range hint
1564-1564
: Add a docstring to thetest_create_model_version_duplicate_fails
method.
Line range hint
1595-1595
: Add a docstring to thetest_update_model_version
method.
Line range hint
1603-1603
: Add a docstring to thetest_list_model_version
method.
Line range hint
1672-1672
: Add a docstring to thetest_delete_model_version_found
method.
Line range hint
1721-1721
: Add a docstring to thetest_delete_model_version_not_found
method.
Line range hint
1733-1733
: Add a docstring to thetest_name_and_description_is_mutable
method.
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.
I like the changes. Thank you for making it less pydantic heavy as well, makes my job a lot easier. 😄 One question though, when it comes to named entities, I thought this approach would also go well with pipelines and pipeline runs too (not sure if I am forgetting about anything else). Is there a reason why we left those out?
Feel free to merge it. Going forward, I think we can implement a simple validator for names like this Pydantic v2 to generalize this even further.
Make sense, we have way more named entities indeed. I'll create a ticket with low prior to revisit this later. |
Describe changes
I added masking using the banned set of symbols (only special characters
\t\n\r\v\f
are considered banned with this PR) for tags, models, artifacts, stacks, and stack components.This impacts only the creation of new entities - old ones with invalid names will remain to work, as before, but new ones cannot be created using banned symbols.
Original motivation: symbols like
\n
would be allowed in API calls, but it breaks the Dashboard subsequently.Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests