model download function bugfix and the README - #1
Conversation
|
|
||
| def get_model_url(self, ctx: mlservicewrapper.core.contexts.ServiceContext): | ||
| return ctx.get_parameter_value("ModelUrl", required=True) | ||
| return ctx.get_parameter_value("https://dl.fbaipublicfiles.com/fasttext/supervised-models/lid.176.bin", required=True) |
There was a problem hiding this comment.
This line is kinda funny--notice how this is a base class for two inheritors? The language detection one is just one of them, and it overrides this function with the hard-coded URL. Because of that, this function actually doesn't get called at all for language detection, and only runs for the vectorization version.
Additionally, this change is renaming the parameter, not changing the return value. So for the vectorization endpoint, it'd actually be searching for an environment variable, for example, named SERVICE_https://dl.fbaipublicfiles.com/fasttext/supervised-models/lid.176.bin, compared to before, where it was SERVICE_ModelUrl.
Suffice to say, you could verify this by adding a print call in there, but this line doesn't actually get called at all for language detection.
| if not os.path.exists(model_path): | ||
| url = self.get_model_url(ctx) | ||
|
|
||
| os.mkdir(model_path.rsplit('/', 1)[0]) |
There was a problem hiding this comment.
Makes sense, but, a few scenarios to think about or test:
- How would this work if the directory already exists?
- What if it doesn't exist, but its parent also doesn't exist?
- Would it work on Windows?
There was a problem hiding this comment.
all the three items should be fix now
daishengyu1993
changed the title
MaJaHa95
left a comment
MaJaHa95
left a comment
There was a problem hiding this comment.
Looks good! Functionality is perfect, I just offered a couple built-in options for doing the steps a tad more cleanly.
|
|
||
| if not os.path.exists(model_path): | ||
| model_parent_path = model_path.rsplit(os.sep, 1)[0] |
There was a problem hiding this comment.
Better, but I can do you one better yet! Check out os.path.dirname. The functionality is the same, but it's purpose-built for doing this exact thing.
| if not os.path.exists(model_path): | ||
| model_parent_path = model_path.rsplit(os.sep, 1)[0] | ||
| if not os.path.exists(model_parent_path): | ||
| os.makedirs(model_parent_path) |
There was a problem hiding this comment.
And same here, functionality is identical, but the exist_ok parameter will eliminate the need for the extra if block.
2 participants
PR Type
model download function Bugfix
Description of the changes
It was identified that the model download failed to complete without parent path exist. This fix the download function with the path exist or not.