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

No pkl extension requirement for model name #164

Merged
merged 6 commits into from
Jan 31, 2020

Conversation

sbaidachni
Copy link
Contributor

I would like to have a model name with no pickle extension

@sbaidachni sbaidachni requested a review from algattik January 31, 2020 00:17
@algattik
Copy link
Contributor

I see the following issues:

  • If there are several files under the model dir, one of them is picked up in a non-deterministic way. A registered model is a logical container for one or more files that make up your model. For example, if you have a model that's stored in multiple files, you can register them as a single model in the workspace. (https://docs.microsoft.com/en-us/azure/machine-learning/how-to-deploy-and-where#registermodel)
  • I could potentially have subdirectories with "." in their names, and the glob() would match their names
  • I could potentially have a model file without "." in the name.

A cleaner approach might be to use Model.get_model_path(), which directly returns the file name, is compact and can be used in multi model deployment. The downside is that it requires the model name to be hard-coded (or we could define an environment variable in the AML Environment).

I think a better approach until we enable Environments, might be to keep the original code and remove the if '.pkl' in file: clause, and add an assertion that exactly one file was found.

@sbaidachni
Copy link
Contributor Author

@algattik what about this one (just updated)? It will work in all cases for single model, and I can use my favorite name now rather than to use undocumented pkl extension:)

Copy link
Contributor

@algattik algattik left a comment

Choose a reason for hiding this comment

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

Interesting approach. Suggesting a change to be more future-proof (only the "end" of the path is documented"

diabetes_regression/scoring/score.py Outdated Show resolved Hide resolved
sbaidachni and others added 3 commits January 31, 2020 10:40
@sbaidachni sbaidachni requested a review from algattik January 31, 2020 19:01
@dtzar dtzar changed the title pkl extension should not be a requirement for the model name No pkl extension requirement for model name Jan 31, 2020
@sbaidachni sbaidachni merged commit 8ae6701 into master Jan 31, 2020
@sbaidachni sbaidachni deleted the sbaydach_fix_model_path branch January 31, 2020 23:38
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.

3 participants