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] Ignore _foreach_values in merge artifacts #1729

Merged
merged 2 commits into from
Feb 20, 2024

Conversation

darinyu
Copy link
Collaborator

@darinyu darinyu commented Feb 12, 2024

_foreach_values is now stored as artifacts for each step but this is not intended for users to consume. In merge artifacts, however, we will try to merge them by default, which could fail if the foreach are dynamic. This PR will make merge artifacts skip the foreach value (at join step). This foreach value will have no meaning after joining anyways, so there should be no impact to the entire flow.

@darinyu darinyu changed the title Ignore _foreach_values in merge artifacts [Ready for review] Ignore _foreach_values in merge artifacts Feb 13, 2024
romain-intel
romain-intel previously approved these changes Feb 13, 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.

Minor nit but LGTM.

metaflow/flowspec.py Outdated Show resolved Hide resolved
@darinyu darinyu force-pushed the fix_merge_artifacts branch from 629ba7f to de12845 Compare February 16, 2024 22:56
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

@savingoyal savingoyal merged commit 05ec0ee into master Feb 20, 2024
34 checks passed
@savingoyal savingoyal deleted the fix_merge_artifacts branch February 20, 2024 23:31
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.

3 participants