Uh oh!
There was an error while loading. Please reload this page.
Fix backfill_job_runner to work with custom executors - #32101
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
o-nikolas
commented
Jun 26, 2023
Hmm, this one is odd, because airflow/airflow/executors/executor_loader.py Lines 119 to 148 in 45bd9c9 @adh-wonolo can you include a full traceback (the one in the description has some strange formatting making it hard to read). And also an example of the executor path you're using? You can change the values if any of them are private, but try keep the formatting as similar as possible. |
o-nikolas
commented
Jun 26, 2023
Also this backfill code has changed since I last touched it for AIP-51. airflow/airflow/jobs/backfill_job_runner.py Lines 543 to 549 in 45bd9c9 @potiuk It looks like you made that change in #30255, do you have any context on that? |
adh-wonolo
commented
Jun 27, 2023
@o-nikolas the executor loads and functions normally for everything other than backfills which you can see in the logs too, its just that when executing airflow/airflow/jobs/backfill_job_runner.py Lines 543 to 549 in 45bd9c9 All that's passed is the name of the Executor (in I'm storing the plugin in Here's the stacktrace: Happy to answer any other questions! |
potiuk
commented
Jun 27, 2023
I looked at it and it loooks like a remnant from an old past that sneaked-in when I was refactoring BaseJob. Seems that the "executor_class" which was stored in the BaseJob was actually a different than "executor_class" that has been used in a number of places to check executor compatibility - and using it from job was simply a mistake. But I also found out that the "job.executor_class" is something of a dead-relic. It has not been used anywhere else (only in one place in tests where it was not really needed any more). So I took the liberty to remove it altogether. I also applied the same fix as you did here @adh-wonolo, but with a small twist (there is no need to get class to run the is_local property - the way python works, if you have an object of the class, you can run class method/property directly on the object and it will use the class one if there is no object property defined. So I will close this one in favour of mine: #32219 |
potiuk
commented
Jun 27, 2023
I also made you co-author of that change @adh-wonolo . thanks for letting us know and providing the fix proposal! |
potiuk
commented
Jun 28, 2023
And merged / marked for 2.6.3 -> once agin thans @adh-wonolo for raising it and providing proposed fix (and thanks @o-nikolas for raising my attantion :). |
adh-wonolo
commented
Jun 28, 2023
Thanks so much @potiuk! Looking forward to collaborate again in the future :) |
Backfill Job Runner pulls in the class name of your executor but doesn't pull in the full path so if you aren't using a default core executor you get an error like:
This can be fixed either by just passing in the actual class to
executor_classor passing in#f"{self.job.executor.__class__.__module__}.{self.job.executor_class}"toExecutorLoader.import_executor_clsor settingself.job.executor_classto be#f"{self.job.executor.__class__.__module__}.{self.job.executor.__class__.__name}"I'm not sure which of these three is the best solution, though in my quick read through the code it seems like this isn't really called elsewhere besides in this specific file.
I ran the core tests and they all passed.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.