metaflow icon indicating copy to clipboard operation
metaflow copied to clipboard

add list runs command

Open hamelsmu opened this issue 3 years ago • 10 comments

~TODO: I need to add tests~

@valayDave @savingoyal this is ready for review

This will close #602

hamelsmu avatar May 09 '22 19:05 hamelsmu

~Oh I need to add the bit about the --quiet flag~

Update: fixed this by using echo instead of echo_always

hamelsmu avatar May 10 '22 19:05 hamelsmu

@savingoyal I was discussing this with @valayDave and some questions:

  1. Do we want to add a --namespace flag as well?
  2. If we want to add 1, should we not have the --user flag and let people pass the namespace user:<username> instead?
  3. If we want to keep both a --namepsace and --user flag, should they be mutually exclusive, or how do we want to handle collisions in this case, should we throw an error if there if there is a --namespace argument with user:<username> etc?

Thanks

hamelsmu avatar May 10 '22 22:05 hamelsmu

How about we add both? It's usually a lot easier as a user for me to do python flow.py list --my runs or python flow.py list --user=hamel runs. We can also support namespace and make the set - my, user and namespace mutually exclusive. wdyt?

savingoyal avatar May 10 '22 22:05 savingoyal

Ok. Here are some key questions that would need decisions to quickly converge on code changes.

  1. What are the levels of filters were wish to support. Some I can think of are: tags, namespace, user, date, branch/project, success/failure.
  2. When options for these filters are provided from CLI, then will the resultant runs be filtered based on the union of filters or will there be some compatibility Constraints. An example constraint: You can provide date or tags but not both. Not saying we need to place this constraint. Just giving an example.
  3. Should the namespace command be list or something else ? Since we right now have something like card list or batch list-jobs etc. Similarly why not runs list. I agree that it is quite close to run making it a little confusing for someone at some point. And if we are adding list then should we add cards and others under this namespace too?
  4. Given the convergence on the above decisions, what is the final command spec. (Example command with all arguments)

valayDave avatar May 10 '22 22:05 valayDave

@savingoyal I have added the --namespace flag so that we have something to look at and consider, but I think we should definitely discuss @valayDave 's comments above. Please LMK your thoughts

hamelsmu avatar May 10 '22 23:05 hamelsmu

The namespace is implicitly the user (which I thought was the initial intent) but yes, this is exactly the type of conversation I was mentioning we should have around this more general "listing" API. These types of higher-level options (independent in effect of whether you are listing runs, steps, artifacts, etc) is useful.

romain-intel avatar May 11 '22 01:05 romain-intel

ignore this comment (duplicate caused by weird browser glitch

hamelsmu avatar May 11 '22 14:05 hamelsmu

ok @romain-intel I made the changes (If I understood correctly) in this commit

I made the following changes:

  1. removed the --my-runs flag and made that the default behavior
  2. added a --all flag that calls namespace(None) to allow you to access the global namespace
  3. Filtered runs by calling namespace(<namespace>), instead of iterating through tags of each run

Please let me know if I am understanding correctly, hopefully, I'm getting closer!

We still have to have a discussion of this interface in general. Note: this PR was also meant as a "Good First Issue" for me tackle with regards to learning the code base of Metaflow (just to let you know the dual purpose of this PR)

hamelsmu avatar May 11 '22 14:05 hamelsmu

Feedback from @savingoyal

My take is that we can expand list in the future to include cards etc. and supporting just --namespace (and maybe --my and --user as well) for now is a good start. We can always introduce more flags as we learn usage patterns. --namespace already takes care of tags , user , date , project etc.

re: --my-runs feedback from @savingoyal:

I shared with Savin that I removed the --my-runs flag per @romain-intel 's suggestion and added the --show-all flag and he thinks this is 👍🏽 , with one minor suggestion:

can we do list --all runs instead of list --show-all runs

hamelsmu avatar May 11 '22 20:05 hamelsmu

Converted the PR to draft since it is not ready for review yet.

savingoyal avatar May 25 '22 19:05 savingoyal