SEPURI-SAI-KRISHNA commented on code in PR #76739:
URL: https://github.com/apache/airflow/pull/76739#discussion_r4512769741
##########
providers/papermill/src/airflow/providers/papermill/operators/papermill.py:
##########
@@ -142,6 +148,28 @@ def execute(self, context: Context):
**remote_kernel_kwargs,
)
+ if self.nbconvert:
Review Comment:
Thanks, this is a good catch and you are right that the remote case is a
real problem. papermill registers handlers for `s3://`, `gs://`, `hdfs://`,
`abs://` and `adl://`, so `output_nb` can well be a URL that `jupyter
nbconvert` cannot open.
I ended up going a slightly different way than skipping, and I would be glad
to hear what you think. My hesitation with skipping is that `nbconvert=True`
then quietly does nothing, which is the same shape as the bug this PR is trying
to fix, and a warning in a task log is easy to miss. The part of your point I
wanted to keep is that the failure should not arrive after the notebook has
already run, since the retry then repeats all of it. Validating before
`pm.execute_notebook` seemed to cover that concern directly, so nothing is
wasted either way and the bad configuration still gets reported.
So `execute` now checks up front and raises `ValueError` naming the URL,
suggesting either a local output notebook or `nbconvert=False`. I also moved
the `nbconvert_args` type check into the same pre-flight, since it had the same
late-failure problem.
Looking into your case turned up two more things I had missed, so thank you
for that. A `file://` URL needs rejecting too, since nbconvert cannot read it
either, so the check now looks at the URL scheme rather than at `"://"`,
treating a single character scheme as a Windows drive letter. And `nbconvert`
is itself in `template_fields`, so `nbconvert="{{ params.convert }}"` resolving
to `False` arrived as the string `"False"`, which is truthy, and would have
turned the conversion on for someone who asked for it off. It now goes through
`airflow.utils.strings.to_boolean` when it arrives as a string.
On tests, the remote rejection is parametrized over `s3://`, `gs://`,
`hdfs://` and `file://`, each asserting `pm.execute_notebook` was never called,
plus one that a remote `output_nb` without `nbconvert` still runs normally, and
one that a rendered `nbconvert` is read as a boolean over `True`, `False`,
`"True"`, `"False"` and `""`. I reverted each change in turn to confirm the
matching test fails without it. The file is at 25 passed.
Thanks also for offering the commit. I wrote it out directly rather than
cherry picking, mostly because it drifted from your version once the scheme and
boolean handling went in, but the report is yours and it clearly improved the
PR. If you or a committer would prefer the skip semantics after all, I am happy
to switch.
--
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]
