Skip to content

Conversation

@hamelsmu
Copy link

@hamelsmu hamelsmu commented May 9, 2022

TODO: I need to add tests

@valayDave @savingoyal this is ready for review

This will close #602

@hamelsmu hamelsmu marked this pull request as draft May 9, 2022 19:29
@hamelsmu hamelsmu marked this pull request as ready for review May 10, 2022 18:39
@hamelsmu hamelsmu requested review from savingoyal and valayDave May 10, 2022 18:40
Copy link
Contributor

@romain-intel romain-intel left a comment

Choose a reason for hiding this comment

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

I don't have a huge issue with this PR but it does seem somewhat of a point solution and maybe we should think about the "list" API a bit more generally. It may be that we want to add more types of "lists" after (not saying to do it now but at least to think about it a bit).

@hamelsmu
Copy link
Author

hamelsmu commented May 10, 2022

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

Update: fixed this by using echo instead of echo_always

counter = 1
try:
flow = Flow(flow_name)
except MetaflowNotFound:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to set the correct namespace here to ensure that the Flow or Run can be found.

Copy link
Author

Choose a reason for hiding this comment

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

@romain-intel perhaps you can shed some light here as I think I am confused b/w what Savin is saying and what you are saying elsewhere?

Happy to have a sync conversation as well around this. Thank you both for your help

@hamelsmu
Copy link
Author

hamelsmu commented May 10, 2022

@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

@savingoyal
Copy link
Collaborator

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?

@valayDave
Copy link
Collaborator

valayDave commented May 10, 2022

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)

@hamelsmu
Copy link
Author

hamelsmu commented May 10, 2022

@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 hamelsmu requested a review from savingoyal May 10, 2022 23:42
@romain-intel
Copy link
Contributor

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.

@hamelsmu
Copy link
Author

hamelsmu commented May 11, 2022

ignore this comment (duplicate caused by weird browser glitch

@hamelsmu
Copy link
Author

hamelsmu commented May 11, 2022

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
Copy link
Author

hamelsmu commented May 11, 2022

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

@savingoyal savingoyal marked this pull request as draft May 25, 2022 18:59
@savingoyal
Copy link
Collaborator

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

@savingoyal savingoyal closed this Aug 30, 2022
@savingoyal savingoyal deleted the list-runs branch September 3, 2022 18:36
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.

Feature Request : Printing information about recent runs via CLI

5 participants