-
Notifications
You must be signed in to change notification settings - Fork 788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
initial runner api #1732
initial runner api #1732
Conversation
I'm not going to comment on the implementation (which is fairly straightforward at this time anyways) because I want to align on some UX things first. These are my initial thoughts on this but feel free to disagree with anything:
Not saying something exactly like this but this more cleanly maps to what we have for click and doesn't feel too un-natural (you have a Cli object -- or call it something else -- and from there you can run with various options exactly like you would on the command line). We can separately argue if we want to infer "environment" for example but that's a separate conversation :). Anyways, hopefully this shows the idea of having a multi level approach that maps more closely to click and can support any arbitrary options. It could be a very thin layer and should be reusable for various commands.
(note this is probably not a good idea since it can loop forever but you get the idea). We should also seriously revisit some of these status things (we had a long conversation in the past about this but it may become more useful as part of this as well -- for example, we can actually return a better "finished" value here because we know if the subprocess dies/goes away and so it may not run forever :)).
|
The usage looks something like this, we can of-course pass
or perhaps...
All this is built on top of a low-level chaining API that is dynamically constructed while traversing through the CLI command tree... Please see the following for usage:
|
Pushed out a subprocess manager that allows us to tail logs via streaming them into a queue first... An example of how the higher level UX is as follows:
the output looks like:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looked at the process manager for now.
The latest usage looks like this:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial comments. WIll need a bit more time but looks promising :)
metaflow/subprocess_manager.py
Outdated
else: | ||
line = await asyncio.wait_for(f.readline(), timeout_per_line) | ||
except asyncio.TimeoutError as e: | ||
raise LogReadTimeoutError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could also return the position and None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we would return:
- the actual line
- an empty line if EOF or just an actual empty line
- None if a timeout.
I don't know if we want to distinguish the cases in 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can discuss for more clarity...
New Higher Level UX:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked all the logic in click_api.py but if it works for the runner, it should be good enough for now. We can improve as we go along and expose more of it.
The Usage looks something like this:
where the contents of
try.py
for the example are:In the above example, the runs are executed 2 at a time. The order of them being completed is:
The total running time is 60 seconds. This is as follows:
Assumptions:
Controlling
and notAuthoring
metaflow runs... at least for now...FlowSpec
Things that need to be discussed
--with
, etc.)Pool
, what about justRun
?Eg: this API is used as a step in some flow to produce an artefact which is consumed and used by another step