simi commented on PR #55894: URL: https://github.com/apache/airflow/pull/55894#issuecomment-3409963103
> Again - if I am right - maybe a better solution that is both DRY and side-effect frree for regular DagBag usage, might be another class (DagBagWithExtraBundlePath - I know, bad name) which would extend from the DagBag, do the try/finally around DagBag parse in it's own parse - and use that class specifically in those places that need to extend sys.path. > The "orignal" way (if I interpret it correctly) addded the sys.path entry not in the "DagBag" - but outside - in the code that we "knew" we don't care about side-effects (for example in dag file processor parsing is done in a forked interpreter, and the interpreter exists after the Dag is serialized). What would be the difference? `sys.path` will get mangled anyway (if used in non-forked env). Are those 3 cases handled in this PR this kind of "fire and forget the environment" situation? - `dag_command.py` seems yes to me, it does the work and exit - `processor.py` not sure - `task_runner.py` not sure IMHO Approach in this PR even makes those places having the original append only better, since it includes the cleanup also now. > cleanup code is also messier than I’d like @uranusjr please suggest better cleanup strategy, happy to improve -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
