Skip to content

Support for multi-flow decorators.#1224

Closed
valayDave wants to merge 4 commits intomasterfrom
valay/multi-flow-decos
Closed

Support for multi-flow decorators.#1224
valayDave wants to merge 4 commits intomasterfrom
valay/multi-flow-decos

Conversation

@valayDave
Copy link
Copy Markdown
Collaborator

@valayDave valayDave commented Jan 9, 2023

PR Stack Merge Order (#1252 --> #1234 --> #1226 --> #1225 --> #1224)

  • This change is required to support Airflow Sensors.
  • Airflow Sensors will support multiple flow decorators per flow
  • This will also be the base PR for the stacked Airflow Foreach + Sensors + Airflow GCP Support PRs

savingoyal
savingoyal previously approved these changes Jan 19, 2023
Copy link
Copy Markdown
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.

A few minor nits.

flow, graph, environment, flow_datastore, metadata, logger, echo, opts
)
for rd in deco:
opts = {option: deco_options[option] for option in rd.options}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be hoisted out of the inner for loop because it makes it look like there are different options for each rd which is not the case (they are shared for a given type of decorator).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Won't the decorator options be different per decorator instance. For example :

@airflow_sensor_a(myopt1="DEF")
@airflow_sensor_a(myopt1="ABC")
class MyFlow(FlowSpec):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, but those are attributes and not options (I got confused too). The attributes are things extracted from the decorator itself and the optiosn are extracted from the command line (so for project, for example, you have name as an attribute and production and branch as options).

To remove this confusion, when we were talking with @savingoyal today, we thought that maybe we could assert that if allow_multiple is true, the options dict is empty. An even better way of doing it is ensuring this at runtime too but it's a bit more complicated (ie: ensuring that it can't be filled in later too). This will prevent some weird things and ppl getting surprised.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Refactored it. Ready for review.

romain-intel
romain-intel previously approved these changes Jan 28, 2023
Copy link
Copy Markdown
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. Gimme until monday to make my changes and test it out but should be good.

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