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

Make execution_date_or_run_id optional in tasks test command #26114

Merged

Conversation

dstandish
Copy link
Contributor

No description provided.

@dstandish dstandish requested a review from kaxil September 1, 2022 16:51
@dstandish dstandish changed the title Make execution_date_or_run_id an optional argument for tasks test Make execution_date_or_run_id optional tasks test command Sep 1, 2022
Comment on lines 180 to 185
ARG_EXECUTION_DATE_OR_RUN_ID = Arg(
('execution_date_or_run_id',), help="The execution_date of the DAG or run_id of the DAGRun"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you not need to delete this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's used elsewhere

@ashb
Copy link
Member

ashb commented Sep 1, 2022

What's the reason for this? Because right now everything in Airflow task execution is for a specific date,

So given airflow tasks test example_python_operator print_the_context what will it print for logical_date, or data_interval_start?

Do any docs need updating too?

@dstandish
Copy link
Contributor Author

dstandish commented Sep 1, 2022

What's the reason for this? Because right now everything in Airflow task execution is for a specific date,

Right, it's always "there" for airlfow, at least internally. But often times as a task developer you don't care about the execution_date. E.g. when catchup=False, probably you aren't really concerned with it. For those cases, it's nice to just be able to run the task and not have to pick an arbitrary date you don't care about, just so you can run your task via CLI.

So given airflow tasks test example_python_operator print_the_context what will it print for logical_date, or data_interval_start?

Should be same as if you put today UTC in for execution_date.

Do any docs need updating too?

i'll take a look.

@dstandish dstandish force-pushed the execution-date-optional-tasks-test branch from 0a90ae3 to cdb1f39 Compare September 1, 2022 21:29
@dstandish dstandish changed the title Make execution_date_or_run_id optional tasks test command Make execution_date_or_run_id optional in tasks test command Sep 1, 2022
@dstandish dstandish force-pushed the execution-date-optional-tasks-test branch 2 times, most recently from 2040fb2 to efb1fa6 Compare September 2, 2022 13:47
@dstandish dstandish force-pushed the execution-date-optional-tasks-test branch from efb1fa6 to c5a5137 Compare September 2, 2022 16:43
@dstandish
Copy link
Contributor Author

did make the small adjustments required in the docs. and i think it will build green now -- was just a static check remaining.

@dstandish dstandish dismissed dimberman’s stale review September 6, 2022 16:35

resolved the comment

create_if_necessary: CreateIfNecessary,
session: Session,
exec_date_or_run_id: Optional[str] = None,
session: Session = NEW_SESSION,
Copy link
Member

Choose a reason for hiding this comment

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

If we make this default session change, should we also @provide_session this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't see why the one should prompt the other. the NEW_SESSION change doesn't really change the behavior of the function. the reason it's there in general is this:

# A fake session to use in functions decorated by provide_session. This allows
# the 'session' argument to be of type Session instead of Optional[Session],
# making it easier to type hint the function body without dealing with the None
# case that can never happen at runtime.

but for me, i'm adding it here simply because i want to keep it as the last argument, so to do so, it needs a default, since i'm making exec date optional.

but what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

All arguments in this function are keyword-only, so ordering does not matter. You can add the new argument anywhere (but behind *) and keep all other arguments as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ashb has previously requested that we keep session as the last argument 🤷

@ashb ?

Copy link
Member

Choose a reason for hiding this comment

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

You can keep it last, keyword-only means you don’t need to add a default to keep it last.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤯

@jedcunningham jedcunningham merged commit 243b3d7 into apache:main Sep 7, 2022
@jedcunningham jedcunningham deleted the execution-date-optional-tasks-test branch September 7, 2022 22:02
@ephraimbuddy ephraimbuddy added the type:new-feature Changelog: New Features label Sep 13, 2022
@ephraimbuddy ephraimbuddy added this to the Airflow 2.4.0 milestone Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI type:new-feature Changelog: New Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants