Skip to content
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

Artifact string name validation #817

Merged
merged 3 commits into from Jun 19, 2020
Merged

Conversation

AlexDut
Copy link
Contributor

@AlexDut AlexDut commented Jun 18, 2020

Description

Add simple validation for artifact's 'name' attribute. As this name is used as a property to access the artifact, the string should respect Python's variable naming conventions. Those are the following:

  • A variable name must start with a letter or the underscore character.
  • A variable name cannot start with a number.
  • A variable name can only contain alpha-numeric characters and underscores.
  • Variable names are case-sensitive.

The check is done in BentoServiceArtifact init method.
Some tests were added for the validation.

Motivation and Context

Issue #747

How Has This Been Tested?

Following the DEVELOPMENT.md file:

$ ./travis/unit_tests.sh

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Documentation
  • Test, CI, or build
  • None

Components (if applicable)

  • BentoService (model packaging, dependency management, handler definition)
  • Model Artifact (model serialization, multi-framework support)
  • Model Server (mico-batching, logging, metrics, tracing, benchmark, OpenAPI)
  • YataiService (model management, deployment automation)
  • Documentation

Checklist:

  • My code follows the bentoml code style, both ./dev/format.sh and
    ./dev/lint.sh script have passed
    (instructions).
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • My change requires a change in bentoml/gallery example notebooks
  • I have sent a pull request to bentoml/gallery to make that change

@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #817 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #817      +/-   ##
==========================================
+ Coverage   56.38%   56.39%   +0.01%     
==========================================
  Files         116      116              
  Lines        8603     8605       +2     
==========================================
+ Hits         4851     4853       +2     
  Misses       3752     3752              
Impacted Files Coverage Δ
bentoml/artifact/artifact.py 90.00% <100.00%> (+0.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e40d764...0bd0620. Read the comment docs.

BentoServiceArtifact(name)
assert "'name' must start with a letter" in str(e.value)

name = "_"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this case does fit the requirement description: 'name' must start with a letter (a-z|A-Z) or underscore character and can only contain alpha-numeric characters and underscores

Could you take a look and see if the benotoml.utils.isidentifier method work for this check?

Copy link
Member

@parano parano Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, since we dropped support for PY2, we can probably just use the name.isidentifier() here. The error message could be improved as well, e.g.:

"Artifact name must be a valid python identifier, a string is considered a valid identifier if it only contains alphanumeric letters (a-z) and (0-9), or underscores (_). A valid identifier cannot start with a number, or contain any spaces."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the regex to name.isidentifier() and the error message with the one you suggested.
Btw, benotoml.utils.isidentifier function was failing for the check and the check with accents (same for name.isidentifier()).
Those, 2 tests were not fitting the requirements of a python identifier. I was a bit to restrictive when I read the definition and actually you can define variables names with accents or using the Underscore(_) character even if it is not recommended.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the PR @AlexDut, surprised that characters with accents work for isidentifier, but I think technology as long as it is a valid python identifier we can use it as a property name for accessing the artifact.

@parano parano added the LGTM label Jun 19, 2020
@parano parano merged commit 7a08cc3 into bentoml:master Jun 19, 2020
aarnphm pushed a commit to aarnphm/BentoML that referenced this pull request Jul 29, 2022
* Add artifact name validation

* Format code for linter

* Use isidentifier + Better error message

Co-authored-by: Alexis Dutot <alexis.dutot@linkfluence.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants