-
Notifications
You must be signed in to change notification settings - Fork 54
Bump Airflow to 2.4.0, standardize version bump process #737
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the testing instructions and ran phylopic, and all went well.
@@ -92,7 +92,7 @@ | |||
_DATE_RANGE_INNER_TEMPLATE = "macros.ds_add(ds, -{} )" | |||
DATE_RANGE_ARG_TEMPLATE = "{{{{" + _DATE_RANGE_INNER_TEMPLATE + "}}}}" | |||
DATE_PARTITION_ARG_TEMPLATE = Template( | |||
"$media_type/$provider_name/{{ date_partition_for_prefix(dag.schedule, dag_run.logical_date, $reingestion_date ) }}" # noqa | |||
"$media_type/$provider_name/{{ date_partition_for_prefix(dag.schedule_interval, dag_run.logical_date, $reingestion_date ) }}" # noqa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so odd! I thought maybe dag.params['schedule']
might work, but no:
ERROR - Failed to execute task: 'airflow.models.param.ParamsDict object' has no attribute 'schedule'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely made me do a double take at first 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Built and ran the legacy smk script and everything looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I tested with Cleveland, Jamendo, and Wikimedia Reingestion. I tried to test the audio data refresh but am unable to get it working due to environment problems; if I get those sorted I'll test that as well, but I see no reason it shouldn't work 🙂
The additions to ignore warnings are very nice (and individually well documented)! I got no warnings, after I updated my local .env
. Shame about dag.schedule_interval
.
Edit: forgot to add that the only other instances of schedule_interval
I found were in several DAGs in the retired
dir. They're retired so we certainly don't need to change them. What do you think @AetherUnbound?
# Print the current Airflow version | ||
@airflow-version: | ||
echo $PROJECT_AIRFLOW_VERSION | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 💯
Ah, great thought on the retired DAGs. I'll revert those changes, we definitely don't need to change them. I used too aggressive a find-and-replace 😅 |
c5df90e
to
1f1043c
Compare
Fixes
Fixes WordPress/openverse#1582 by @AetherUnbound
This also addresses some security issues that dependabot identified.
Description
This PR updates our Airflow version to 2.4.0. I've also borrowed the pattern set up in #656 and applied it to the Airflow version, so we only have to update it in one place now!
As part of the version update, I've also updated the
schedule_interval
parameter to beschedule
instead. What's unfortunate however is that theairflow.models.dag.Dag
model does not have aschedule
attribute, you still must useschedule_interval
when referencing it on the DAG object. So there are a few instances where we were not able to update that value.I added a bunch of deprecation warning filters for our tests to make the warning summary more relevant (presently there should be no warnings). Most of the warnings were either 1) irrelevant & unfixable or 2) due to upstream dependencies.
Lastly, and probably most notably:
Testing Instructions
just build
just test --extended
(and ensure there are no warnings in the warnings summary)just airflow-version
should report2.4.0
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin