Add bundle_path temporarily to sys.path during DagBag parsing. #55894
Conversation
simi
requested review from
XD-DENG,
amoghrajesh,
ashb,
ephraimbuddy,
jedcunningham and
kaxil
as code owners
simi
force-pushed
the
dagbag-syspath
branch
2 times, most recently
from
5cc0b1c to
c07ffef
Compare
|
This is also affecting 3.0 right? (That is, this isn't a new regression in 3.1?) |
IMHO this was missed in 3.0.2 during some Task API refactorings (moving to DAG bundles). 3.0.1 works the same as 3.1 with this patch. |
|
3.0.0/3.0.1 was arguably worse in the sense it naively added the AF2 dags dir instead of bundle root, even if it wasnt configured. But yes, not a regression. Meaning, it should go in rc2 if merged and we have an rc2, but not enough to warrant an rc2 on its own. |
That's my understanding also. 3.0.0 and 3.0.1 was actually wrong and this is actually improvement to bundle path to be appended when needed automatically so you doesn't need to do it manually on all obvious cases. |
simi
force-pushed
the
dagbag-syspath
branch
from
c07ffef to
400adf6
Compare
|
@jedcunningham would you mind to help me resolve the one comment? I have no idea what's requested there and rc2 is around the corner. 🙏 |
|
Sorry, thought I'd replied to the task thing, I assume thats what you meant? Shout if you have other questions. (Missed rc2 though) |
simi
force-pushed
the
dagbag-syspath
branch
from
400adf6 to
15bf97e
Compare
|
@jedcunningham I'm not sure I do follow |
|
Those are literal |
Ahh, I had no idea bout |
simi
force-pushed
the
dagbag-syspath
branch
from
15bf97e to
6628ef0
Compare
|
updated @jedcunningham |
simi
force-pushed
the
dagbag-syspath
branch
2 times, most recently
from
5ee682a to
00a1cb4
Compare
|
A couple of things we should fix, but otherwise I think this is good! |
uranusjr
removed
stale
- it is intended to pass it to DagBag only for ephemeral processes
- usefor CLI and other one-off DagBag instances 'polluting' sys.path
|
@uranusjr update. Not sure if the warning is not too much. I did it this way to keep the kw args to BundleDagBag the same as DagBag. Looking at the usage, maybe it is not worth it and it will be better to strip only to currently used kw params and provide safe defaults for the others. |
simi
force-pushed
the
dagbag-syspath
branch
2 times, most recently
from
f196d7b to
d6df95f
Compare
|
Agree with your latest comment, but we’ll need to do this for compat reasons. Feel free to submit refactoring PRs after this if you feel like reworking it; we can discuss what needs to be done there. |
|
Good to squash now and update the commit message? |
No need. We use "squash & merge" as the only way to merge PRs so they are always squashed into single commit when we merge. |
|
🥳 Thanks everyone for a guidance and patience! It is actually my 🎂 today, this is nice accidental 🎁! |
|
Happy BDay ! Glad we could make it better! |
…e#55894) * Add bundle_path temporarily to sys.path during DagBag parsing. * Add bundle_path to sys.path permanently. - it is intended to pass it to DagBag only for ephemeral processes * Introduce BundleDagBag for ephemeral pre-loaded bundle path usage. - usefor CLI and other one-off DagBag instances 'polluting' sys.path * Remove from backwards compatible layer. Ignore include_examples.
7 participants
This fixes various entrypoints to DAGs where path wasn't updated like various cli DAG commands like
airflow dags test.I have tested various entrypoints locally - using full Airflow deployment, using local standalone airflow, running DAGs thru UI, running DAGs thru CLI, serializing DAGs thru processor, serializing DAGs thru CLI.
/cc @potiuk