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

[Ready for Review] fix bug where client can not access foreach stack #1766

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

Conversation

darinyu
Copy link
Collaborator

@darinyu darinyu commented Mar 14, 2024

To reproduce, have a flow with foreach task, and then in python client, do

from metaflow import Task
>>> task = Task("ForeachFlow/123/foreach_step/task-00000000")
>>> task.index
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/root/metaflow/metaflow/client/core.py", line 1166, in index
    return self["_foreach_stack"].data[-1].index
  File "/root/metaflow/metaflow/client/core.py", line 875, in data
    obj = filecache.get_artifact(ds_type, location[6:], meta, *components)
  File "/root/metaflow/metaflow/client/filecache.py", line 207, in get_artifact
    _, obj = next(
  File "/root/metaflow/metaflow/datastore/task_datastore.py", line 370, in load_artifacts
    yield name, pickle.loads(blob)
AttributeError: Can't get attribute 'ForeachFrame' on <module '__main__' (built-in)>

This is because this ForeachFrame is loaded to main, and defined in task.py. Without a flow, this ForeachFrame is not imported. This PR refactors the namedtuple util to top level (with minimum dependency) and import it at top level.

@darinyu darinyu changed the title fix bug where client can not access foreach stack [Ready for Review] fix bug where client can not access foreach stack Mar 14, 2024
romain-intel
romain-intel previously approved these changes Mar 14, 2024
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.

LGTM with minor comment to not introduce a new symbol.

@@ -143,6 +143,9 @@ class and related decorators.
DataArtifact,
)

# Data class needed for Client.
from .tuple_util import ForeachFrame
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, you probably don't need to import ForeachFrame. You can probably simply do: from . import tuple_util

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@darinyu darinyu changed the title [Ready for Review] fix bug where client can not access foreach stack [WIP] fix bug where client can not access foreach stack Apr 8, 2024
@romain-intel
Copy link
Contributor

@darinyu -- is this no longer something you want merged? I thought it was still relevant.

@romain-intel
Copy link
Contributor

@darinyu for when you are back :)

@darinyu darinyu force-pushed the fix_foreach_frame_in_client branch from 0cba69b to 36eb274 Compare May 9, 2024 04:30
@darinyu darinyu changed the title [WIP] fix bug where client can not access foreach stack [Ready for Review] fix bug where client can not access foreach stack May 9, 2024
@darinyu
Copy link
Collaborator Author

darinyu commented May 9, 2024

Yeah this still needs to go in, it slipped during before my vacation. I just rebased this commit and I think it is ready to go in now.

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 think this looks good. I think we should be good to merge.

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.

None yet

3 participants