Skip to content
This repository has been archived by the owner on Sep 13, 2023. It is now read-only.

Add -c help WIP #363

Merged
merged 41 commits into from
Sep 8, 2022
Merged

Add -c help WIP #363

merged 41 commits into from
Sep 8, 2022

Conversation

mike0sv
Copy link
Contributor

@mike0sv mike0sv commented Jul 25, 2022 •
edited by aguschin
Loading

I was investigating how we can add help to -c options. I was able to do this (but in a bit nasty way)
You can check it out with mlem declare2 env docker --help. Should work with other mlem declare2 commands.
I didnt add field types, defaults and support for complex/nested objects yet
cc @aguschin @jorgeorpinel @omesser

close #214
close #233
close #351

@mike0sv mike0sv temporarily deployed to internal July 25, 2022 23:40 Inactive
@aguschin aguschin mentioned this pull request Aug 2, 2022
Closed
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Aug 2, 2022 •
edited
Loading

I guess this is related to #233 ? If so cc @dberenbaum

I'm not sure I'm getting this mlem declare2 syntax TBH. Maybe a bit more explanation or quick demo in the PR description?

jorgeorpinel
jorgeorpinel reviewed Aug 2, 2022
View reviewed changes
mlem/cli/declare.py Outdated Show resolved Hide resolved
@mike0sv
Copy link
Contributor Author

mike0sv commented Aug 8, 2022

Some examples:
mlem declare --help
image
mlem declare env --help
image
mlem declare env docker --help
image

@mike0sv
Copy link
Contributor Author

mike0sv commented Aug 9, 2022

Now it looks like this
image

@mike0sv mike0sv temporarily deployed to internal August 9, 2022 15:27 Inactive
@mike0sv
Copy link
Contributor Author

mike0sv commented Aug 9, 2022

Made less dirty and added for other commands
image
image

This breaks backward compatibility for some of them because of rearranged arguments (mlem serve model type -> mlem serve type model)
Also we'll need to fill all docstrings for classes and their fields
Also #351 is still an issue as well as this: some models has __root__ field and we need to handle this

@aguschin

This comment was marked as resolved.

Sign in to view
@mike0sv mike0sv temporarily deployed to internal August 10, 2022 14:35 Inactive
@mike0sv

This comment was marked as resolved.

Sign in to view
@mike0sv mike0sv temporarily deployed to internal August 16, 2022 15:37 Inactive
@mike0sv mike0sv temporarily deployed to internal August 16, 2022 15:38 Inactive
@mike0sv mike0sv temporarily deployed to internal August 16, 2022 18:00 Inactive
@mike0sv mike0sv temporarily deployed to internal August 16, 2022 18:27 Inactive
@codecov
Copy link

codecov bot commented Aug 16, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (release/0.3.0@5cef637). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files 
@@               Coverage Diff                @@
##             release/0.3.0     #363   +/-   ##
================================================
  Coverage                 ?   90.59%           
================================================
  Files                    ?       82           
  Lines                    ?     6856           
  Branches                 ?        0           
================================================
  Hits                     ?     6211           
  Misses                   ?      645           
  Partials                 ?        0

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mike0sv mike0sv temporarily deployed to internal August 17, 2022 10:06 Inactive
@mike0sv mike0sv temporarily deployed to internal August 17, 2022 10:15 Inactive
@mike0sv mike0sv temporarily deployed to internal August 17, 2022 10:34 Inactive
@mike0sv mike0sv temporarily deployed to internal September 5, 2022 18:40 Inactive
@mike0sv mike0sv temporarily deployed to internal September 5, 2022 19:15 Inactive
@mike0sv mike0sv temporarily deployed to internal September 5, 2022 19:42 Inactive
@mike0sv mike0sv temporarily deployed to internal September 5, 2022 19:59 Inactive
@mike0sv
allow --load for groups
645c0b6 
add server config into docker build
@mike0sv mike0sv temporarily deployed to internal September 6, 2022 19:44 Inactive
@mike0sv mike0sv temporarily deployed to internal September 6, 2022 20:12 Inactive
@mike0sv mike0sv temporarily deployed to internal September 6, 2022 20:27 Inactive
@mike0sv mike0sv temporarily deployed to internal September 6, 2022 20:49 Inactive
@mike0sv mike0sv temporarily deployed to internal September 6, 2022 21:06 Inactive
jorgeorpinel
jorgeorpinel reviewed Sep 6, 2022
View reviewed changes
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Suggestions and comments/ questions on about half of the files changed (other than tests). Please use what you need and generalize to the rest if possible 🙂

mlem/cli/apply.py Show resolved Hide resolved
mlem/cli/apply.py Show resolved Hide resolved
mlem/cli/build.py Show resolved Hide resolved
mlem/cli/build.py Show resolved Hide resolved
mlem/cli/declare.py Show resolved Hide resolved
mlem/cli/link.py Show resolved Hide resolved
mlem/cli/serve.py Show resolved Hide resolved
mlem/cli/serve.py Show resolved Hide resolved
mlem/contrib/catboost.py Show resolved Hide resolved
mlem/cli/types.py Show resolved Hide resolved
@mike0sv mike0sv temporarily deployed to internal September 7, 2022 00:21 Inactive
@mike0sv mike0sv temporarily deployed to internal September 7, 2022 01:10 Inactive
@mike0sv mike0sv merged commit 1f7bc29 into release/0.3.0 Sep 8, 2022
@mike0sv mike0sv deleted the feature/cli-conf-help branch September 8, 2022 22:39
@mike0sv
Copy link
Contributor Author

mike0sv commented Sep 8, 2022

@jorgeorpinel we kinda need to merge this to unblock other PRs, I will address your feedback in a separate PR though

@mike0sv mike0sv mentioned this pull request Sep 9, 2022
Merged
@jorgeorpinel
Copy link
Contributor

need to merge this to unblock other PRs

Sure! Didn't mean to block. I noticed the PR was approved but sometimes there's still other feedback that can def. wait or be rejected. Up to you!

@aguschin aguschin mentioned this pull request Sep 16, 2022
Closed
@mike0sv mike0sv mentioned this pull request Sep 23, 2022
Merged
@aguschin aguschin mentioned this pull request Oct 7, 2022
Closed
@jorgeorpinel jorgeorpinel added A: docs DEPRECATED Area: user documentation; See github.com/iterative/mlem.ai/labels/A%3A%20docs ux Things that matter for user experience and removed A: docs DEPRECATED Area: user documentation; See github.com/iterative/mlem.ai/labels/A%3A%20docs labels Nov 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jorgeorpinel jorgeorpinel jorgeorpinel left review comments

@aguschin aguschin aguschin approved these changes

Assignees

@mike0sv mike0sv

Labels
ux Things that matter for user experience
Projects
No open projects
MLEM + GTO
Status: Done
Milestone
No milestone
3 participants
@mike0sv @jorgeorpinel @aguschin