Fix docstrings #398
Fix docstrings #398
Conversation
mlem/cli/main.py
Outdated
| help="Path to MLEM model object", | ||
| metavar=PATH_METAVAR, | ||
| ) | ||
| option_data = Option( | ||
| ..., "-d", "--data", help="Path to MLEM data object", metavar=PATH_METAVAR |
There was a problem hiding this comment.
Clearer 👍🏼 though maybe "model/data MLEM object" would be a better phrasing (keep "MLEM object" together). No strong opinion
There was a problem hiding this comment.
IMO MLEM model object is sounds better than model MLEM object.
There was a problem hiding this comment.
Is it .mlem path or path to an actual directory / file that MLEM creates? MLEM model object ideally should be just "Path to model"
There was a problem hiding this comment.
I think it's the path to the .mlem file which is why I suggested (somewhere) to add (.mlem file) just to be extra clear. Or maybe indeed we should avoid "object" completely and just say "data .mlem file"/ ".mlem file for the data".
There was a problem hiding this comment.
No, it's a path to an actual model. E.g. if you have model.pkl and model.pkl.mlem nearby, the argument should be model.pkl (although it should work if one supplies model.pkl.mlem as well).
The thing is, if there is no model.pkl.mlem, MLEM won't understand what it is. So it should be MLEM object (== model.pkl.mlem should exist) for this to work, not just a binary saved with pickle/joblib or csv file saved with pandas.
There was a problem hiding this comment.
What @aguschin said is correct, I just see it the other way around - you should specify path to .mlem file, but omitting .mlem suffix also works. But if somehow you have aaa.mlem that uses bbb.pkl file, you can't specify bbb.pkl - you can use aaa.mlem or aaa. But generally bbb.pkl == aaa, unless you tamper with mlem files manually.
As for what @shcheklein said: I added MLEM Object to imply that it only works with models saved by MLEM. We can omit it, but I think this will make it less clear. We can also do Path to MLEM model probably
There was a problem hiding this comment.
Path to MLEM model and Path to MLEM dataset sounds good to me. More concise than Path to MLEM data object
There was a problem hiding this comment.
I see this is merged but just FYI this is similar to dvc push (and other commands) which targets can be either tracked files/dirs or .dvc files. See e.g. https://dvc.org/doc/command-reference/push for ideas on good phrasing around this.
Codecov ReportBase: 86.91% // Head: 87.81% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## release/0.3.0 #398 +/- ##
=================================================
+ Coverage 86.91% 87.81% +0.90%
=================================================
Files 92 92
Lines 7808 7821 +13
=================================================
+ Hits 6786 6868 +82
+ Misses 1022 953 -69
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
# Conflicts: # mlem/contrib/docker/base.py # mlem/contrib/heroku/meta.py # mlem/core/objects.py
jorgeorpinel
added
the
A: docs
Address docsting feedback from #363