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.

Sign up for GitHub

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

Jump to bottom

Introduce support for event-triggered workflows #1271

Merged
merged 45 commits into from
Apr 25, 2023
Merged

Introduce support for event-triggered workflows #1271

merged 45 commits into from
Apr 25, 2023

Conversation

savingoyal
Copy link
Collaborator

@savingoyal savingoyal commented Feb 13, 2023 •
edited
Loading

Items remaining

  • auto-emit events from flows
  • record execution metadata
  • expose client
  • polish up UX
  • support flow dependency
  • support NATS
  • support event spoofing
  • support mapping context and default maps
  • support finer grained @project attributes
  • doc strings & types

draft docs - https://github.com/Netflix/metaflow-docs/pull/86/files

@romain-intel
Copy link
Contributor

Let me know when this is ready for review. Happy to look and provide feedback whenever you want (I am out on the week of the 19th so responses that week may be delayed).

@savingoyal
Copy link
Collaborator Author

Let me know when this is ready for review. Happy to look and provide feedback whenever you want (I am out on the week of the 19th so responses that week may be delayed).

There shouldn't be much to review but I will give you a heads up.

@romain-intel
Copy link
Contributor

Sounds good. And yes, I will probably defer to you for any and all argo stuff :). It's just the common decorator(s)/parameter changes that I will review so yes, shouldn't take long. If you get that part done first, I can try to implement internally and see if I hit any snags as well (so we have a bit more confidence that we are all good).

@savingoyal savingoyal changed the title WIP: Integrate with Argo Events Introduce support for event-triggered workflows Mar 6, 2023
@savingoyal savingoyal mentioned this pull request Mar 6, 2023
Closed
romain-intel
romain-intel previously requested changes Mar 6, 2023
View reviewed changes
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 have not reviewed any of the airflow/argo/step_functions code. I also need to review more the decorators (wrong computer right now to review based on what we currently have). This is therefore an initial high-level review. Happy to talk more.

metaflow/client/core.py Show resolved Hide resolved
metaflow/client/core.py Outdated Show resolved Hide resolved
metaflow/client/core.py Outdated Show resolved Hide resolved
metaflow/events.py Show resolved Hide resolved
metaflow/client/core.py Outdated Show resolved Hide resolved
metaflow/plugins/__init__.py Outdated Show resolved Hide resolved
metaflow/plugins/__init__.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_events_decorator.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_events_decorator.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_events_decorator.py Outdated Show resolved Hide resolved
romain-intel
romain-intel reviewed Mar 14, 2023
View reviewed changes
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.

More comments but converging :). I'll try to have the alias thing shortly. Also, please add typing and documentation to all public facing APIs and classes in a manner consistent with the rest of the documentation.

metaflow/events.py Outdated Show resolved Hide resolved
metaflow/events.py Outdated Show resolved Hide resolved
metaflow/events.py Outdated Show resolved Hide resolved
metaflow/events.py Outdated Show resolved Hide resolved
metaflow/events.py Show resolved Hide resolved
metaflow/client/core.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_events_decorator.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_events_decorator.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_events_decorator.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_events_decorator.py Outdated Show resolved Hide resolved
jackie-ob
jackie-ob reviewed Mar 15, 2023
View reviewed changes
metaflow/client/core.py Outdated Show resolved Hide resolved
metaflow/events.py Outdated Show resolved Hide resolved
metaflow/events.py Outdated Show resolved Hide resolved
metaflow/events.py Show resolved Hide resolved
metaflow/events.py Show resolved Hide resolved
metaflow/plugins/argo/argo_events_decorator.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_events_decorator.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_events_decorator.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_workflows_decorator.py Show resolved Hide resolved
metaflow/plugins/argo/argo_workflows_decorator.py Show resolved Hide resolved
@jackie-ob jackie-ob mentioned this pull request Mar 20, 2023
Merged
@savingoyal savingoyal mentioned this pull request Mar 29, 2023
Closed
romain-intel added a commit that referenced this pull request Apr 3, 2023
@romain-intel
Namespace check skip feature from #1271
415f054
savingoyal pushed a commit that referenced this pull request Apr 3, 2023
@romain-intel
Namespace check skip feature from #1271 (#1341)
b27f64c
saikonen
saikonen reviewed Apr 5, 2023
View reviewed changes
metaflow/plugins/argo/argo_events_decorator.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_workflows.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_workflows.py Outdated Show resolved Hide resolved
romain-intel
romain-intel reviewed Apr 24, 2023
View reviewed changes
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.

Not done reviewing yet but initial comments to get them to you as fast as possible.

metaflow/client/core.py Show resolved Hide resolved
metaflow/events.py Show resolved Hide resolved
metaflow/plugins/events_decorator.py Outdated Show resolved Hide resolved
metaflow/events.py Outdated Show resolved Hide resolved
metaflow/events.py Outdated Show resolved Hide resolved
metaflow/events.py Show resolved Hide resolved
romain-intel
romain-intel reviewed Apr 25, 2023
View reviewed changes
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.

Ok. I think I am done. I have not reviewed any of the airflow, argo, sfn or kube portions instead focusing on the common changes.

metaflow/events.py Show resolved Hide resolved
metaflow/events.py Outdated Show resolved Hide resolved
metaflow/events.py Outdated Show resolved Hide resolved
metaflow/events.py Outdated Show resolved Hide resolved
metaflow/plugins/events_decorator.py Show resolved Hide resolved
metaflow/plugins/events_decorator.py Outdated Show resolved Hide resolved
metaflow/plugins/events_decorator.py Show resolved Hide resolved
metaflow/plugins/events_decorator.py Show resolved Hide resolved
metaflow/plugins/events_decorator.py Show resolved Hide resolved
metaflow/plugins/events_decorator.py Show resolved Hide resolved
romain-intel
romain-intel reviewed Apr 25, 2023
View reviewed changes
metaflow/events.py Outdated Show resolved Hide resolved
savingoyal and others added 2 commits April 25, 2023 10:45
@savingoyal
final fix
af232ab
@saikonen
fix: event triggering docs (#1372)
df1378e 
* explicitly return None for trigger

* address review comments

* polishing events docstrings

* update event decorator types in docstring
@savingoyal savingoyal dismissed romain-intel’s stale review April 25, 2023 22:09

addressed pending comments

@savingoyal savingoyal merged commit daef4fe into master Apr 25, 2023
@savingoyal savingoyal deleted the event-triggering branch April 25, 2023 22:09
@saikonen saikonen mentioned this pull request Apr 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackie-ob jackie-ob jackie-ob left review comments

@saikonen saikonen saikonen left review comments

@valayDave valayDave valayDave left review comments

@talebzeghmi talebzeghmi talebzeghmi left review comments

@romain-intel romain-intel romain-intel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@savingoyal @romain-intel @talebzeghmi @valayDave @saikonen @jackie-ob