-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
MissingParameterException when generating execution summary #2411
Comments
Nice catch. I'm confused by the example run you have pasted. It looks like the execution summary is still printed? I guess it makes sense to use the same CmdlineParser context managerand put the execution summary generation in there. The damage of global variables it seems. |
Yes, the summary is still printed. BTW, the exit code is 1, not the code for I tried to extend the CmdlineParser context manager to the entire run_with_retcodes, but got multiple test failures in retcodes_test.py and cmdline_test.py having to do with expected return codes. So at least some code (or maybe just some test code?) expects the CmdlineParser to be unavailable during some parts of run_with_retcodes. The PR simply puts the call to _summary_dict inside a CmdlineParser context manager. |
Aha, now I know what code path you are talking about! Now it makes sense. |
Previously you could get an error + stack-trace when luigi is evaluating the exit code. closes #2411
Generating the execution summary may generate a MissingParameterException if a
requires
attempts to access a Config parameter which was specified only on the command line.Example:
This seems to be because the execution summary is generated outside any CmdlineParser context manager in run_with_retcodes. So this should be fairly easy to avoid by extending the entire run_with_retcodes to be within the CmdlineParser cm already there for the
retcode()
config - or if that could cause side effects I am unaware of then a separate context just for the call to _summary_dict.I can attempt a PR for either approach.
May be related to #1964
The text was updated successfully, but these errors were encountered: