Skip to content
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

WIP: Initial commit for spin steps #2036

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

WIP: Initial commit for spin steps #2036

wants to merge 17 commits into from

Conversation

talsperre
Copy link
Collaborator

This PR aims to add support for spinning up "baby" steps in Metaflow by using the client API as an input datastore.

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.

Haven't look at all the logic yet but just a few comments. We can discuss in person. Looks quite promising!

help="Command used to spin up a single task based on the artifacts from "
"a previous run or artifacts provided by the user."
)
@click.argument("step-name")
Copy link
Contributor

Choose a reason for hiding this comment

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

some of these should probably be taken from the start (ie: top-level) options. For spin, we should just ignore them and set them to local. I believe we could override them at that level using the invoked_subcommand that click provides (and checking if it is spin).

metaflow/task.py Outdated
output.init_task()

# How we access the input and index attributes depends on the execution context.
# If spin is set to True, we short-circuit attribute access to getatttr directly
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: getattr

@@ -272,6 +273,8 @@ def index(self) -> Optional[int]:
int, optional
Index of the task in a foreach step.
"""
if self._spin:
return getattr(self, "_spin_index")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: return self._spin_index

@@ -292,6 +295,8 @@ def input(self) -> Optional[Any]:
object, optional
Input passed to the foreach task.
"""
if self._spin:
return getattr(self, "_spin_input")
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

metaflow/task.py Outdated
print("%s failed:" % self.flow, file=sys.stderr)
raise
finally:
# TODO: Persist the output artifacts to local datastore
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a task_finished missing as well (after the persist of artifacts)

)

try:
return __getattr__(self.task.artifacts, name)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not getattr?

f"`{self.step_name}`."
)

raise AttributeError(
Copy link
Contributor

Choose a reason for hiding this comment

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

is this ever reached?

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.

2 participants