Fix rendering parameters in PapermillOperator #28979

Taragolis · 2023-01-17T01:34:12Z

Add missing "parameters" into NoteBook's templated fields and remove redundant loop on inlets/outlets

related: #28977

Taragolis · 2023-01-17T01:34:21Z

cc: @TPapajCin @marvinfretly @nicnguyen3103

Taragolis · 2023-01-17T01:37:46Z

airflow/providers/papermill/operators/papermill.py

may be we should also make this fields mandatory

I think it's OK to check their existence in the constructor.

Previously it checked in execute, no idea why, just assume that is somehow related to Linage

uranusjr · 2023-01-17T01:53:22Z

airflow/providers/papermill/operators/papermill.py

Suggested change

template_fields: ClassVar = {*File.template_fields, "parameters"}

template_fields: ClassVar[str] = {*File.template_fields, "parameters"}

(I don’t think the annotation is actually needed at all though, it should inherit the type from File.)

I guess you mean ClassVar[Sequence[str]] instead of just ClassVar[str]

However even with with this annotation I got

error: Incompatible types in assignment (expression has type "Set[str]", variable has type "Sequence[str]") [assignment]

So I change wrap value to tuple

Can’t use sequence, you need Collection[str]. But changing it to tuple works as well, the difference is minimal.

uranusjr

Looks good, but I want one of the users in the original issue to actually test against real Papermill.

Taragolis · 2023-01-17T10:30:15Z

Looks good, but I want one of the users in the original issue to actually test against real Papermill.

+1

marvinfretly · 2023-01-17T13:50:34Z

How can I try it? I'm currently using airflow using docker image

Taragolis · 2023-01-17T14:29:43Z

How can I try it? I'm currently using airflow using docker image

There is different ways, however according to the fact that changes relevant only for single file you could:

Download associated with this PR: https://raw.githubusercontent.com/apache/airflow/e607c90884da37bebc3d4ca257bd253f31954c14/airflow/providers/papermill/operators/papermill.py
Place it in you DAG directory
Use in your DAG from papermill import PapermillOperator instead of from airflow.providers.papermill.operators.papermill import PapermillOperator

Another options:

Use breeze and switch to this branch
Patch sources: https://github.com/apache/airflow/commit/e607c90884da37bebc3d4ca257bd253f31954c14.patch

marvinfretly · 2023-01-17T15:20:34Z

How can I try it? I'm currently using airflow using docker image

There is different ways, however according to the fact that changes relevant only for single file you could:

Download associated with this PR: https://raw.githubusercontent.com/apache/airflow/e607c90884da37bebc3d4ca257bd253f31954c14/airflow/providers/papermill/operators/papermill.py

Place it in you DAG directory

Use in your DAG from papermill import PapermillOperator instead of from airflow.providers.papermill.operators.papermill import PapermillOperator

Another options:

Use breeze and switch to this branch

Patch sources: https://github.com/apache/airflow/commit/e607c90884da37bebc3d4ca257bd253f31954c14.patch

Thank you. Everything is working properly now. Using v2.5.0

potiuk · 2023-01-20T16:40:08Z

Rebased it as for some reason there was only 1 mergable check.

Taragolis · 2023-01-20T20:04:13Z

Well seems like broken compatibility with 2.3.x, I will check later on as well as will change wrong message for error in the CI (point to 2.2.0 but actually we test on 2.3.0)

potiuk · 2023-01-20T20:09:33Z

Well seems like broken compatibility with 2.3.x, I will check later on as well as will change wrong message for error in the CI (point to 2.2.0 but actually we test on 2.3.0)

Not updated when we bumped it :)

Taragolis · 2023-01-20T20:28:54Z

Not updated when we bumped it :)

I have a look and also found that we have environment variable USE_AIRFLOW_VERSION but we explicit set it to "wheel" (maybe somewhere in the past it make sense). We could use it for generate message or skip message if set to None. I plan to have a look more detail tomorrow

potiuk · 2023-01-20T20:36:02Z

I have a look and also found that we have environment variable USE_AIRFLOW_VERSION but we explicit set it to "wheel" (maybe somewhere in the past it make sense). We could use it for generate message or skip message if set to None. I plan to have a look more detail tomorrow

I believe this is correct (but would be great to verify the logic - so please take a look) - in a number of steps in CI we install airflow and providers from pre-built wheels (to simulate the behaviour of installing wheels from PyPI)

See: https://github.com/apache/airflow/blob/main/IMAGES.rst#building-docker-images-from-current-sources

Building docker images from current sources
===========================================

The easy way to build the CI/PROD images is to use `<BREEZE.rst>`_. It uses a number of optimization
and caches to build it efficiently and fast when you are developing Airflow and need to update to
latest version.

CI image, airflow package is always built from sources. When you execute the image, you can however use
the ``--use-airflow-version`` flag (or ``USE_AIRFLOW_VERSION`` environment variable) to remove
the preinstalled source version of Airflow and replace it with one of the possible installation methods:

* "none" airflow is removed and not installed
* "wheel" airflow is removed and replaced with "wheel" version available in dist
* "sdist" airflow is removed and replaced with "sdist" version available in dist
* "<VERSION>" airflow is removed and installed from PyPI (with the specified version)

For PROD image by default production image is built from the latest sources when using Breeze, but when
you use it via docker build command, it uses the latest installed version of airflow and providers.
However, you can choose different installation methods as described in
`Building PROD docker images from released PIP packages <#building-prod-docker-images-from-released-packages>`_.
Detailed reference for building production image from different sources can be found in:
`Build Args reference <docs/docker-stack/build-arg-ref.rst#installing-airflow-using-different-methods>`_

You can build the CI image using current sources this command:

Taragolis · 2023-01-21T09:27:28Z

I believe this is correct

Yes it is. I filtered only .py files when tried to find usage of USE_AIRFLOW_VERSION 🤦 🤦‍♂️ 🤦‍♀️

Taragolis · 2023-01-22T18:31:19Z

Well it was not straight forward to fix this in 2.3 due to cattrs, however finally it should be compatible with 2.3:

Taragolis requested review from eladkal, potiuk and uranusjr January 17, 2023 01:34

boring-cyborg bot added the area:providers label Jan 17, 2023

Taragolis commented Jan 17, 2023

View reviewed changes

uranusjr reviewed Jan 17, 2023

View reviewed changes

potiuk approved these changes Jan 17, 2023

View reviewed changes

Taragolis force-pushed the fix-papermill-operator-rendering branch 2 times, most recently from 6957f2f to e607c90 Compare January 17, 2023 10:09

uranusjr approved these changes Jan 17, 2023

View reviewed changes

potiuk force-pushed the fix-papermill-operator-rendering branch from e607c90 to b79b9c2 Compare January 20, 2023 16:39

Fix rendering parameters in PapermillOperator

37cbb15

Taragolis force-pushed the fix-papermill-operator-rendering branch from b79b9c2 to 37cbb15 Compare January 22, 2023 18:24

potiuk approved these changes Jan 22, 2023

View reviewed changes

potiuk merged commit 736f2e8 into apache:main Jan 22, 2023

eladkal mentioned this pull request Jan 23, 2023

Status of testing Providers that were prepared on January 23, 2023 #29103

Closed

8 tasks

Taragolis mentioned this pull request Sep 28, 2023

Remove Airflow 2.3 compat from PapermillOperator #34675

Merged

Taragolis deleted the fix-papermill-operator-rendering branch September 28, 2023 18:43

	template_fields: ClassVar = {*File.template_fields, "parameters"}
	template_fields: ClassVar[str] = {*File.template_fields, "parameters"}

Fix rendering parameters in PapermillOperator #28979

Fix rendering parameters in PapermillOperator #28979

Uh oh!

Conversation

Taragolis commented Jan 17, 2023

Uh oh!

Taragolis commented Jan 17, 2023

Uh oh!

Taragolis Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

potiuk Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

Taragolis Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

uranusjr Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

Taragolis Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

uranusjr Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

uranusjr left a comment

Choose a reason for hiding this comment

Uh oh!

Taragolis commented Jan 17, 2023

Uh oh!

marvinfretly commented Jan 17, 2023

Uh oh!

Taragolis commented Jan 17, 2023

Uh oh!

marvinfretly commented Jan 17, 2023

Uh oh!

potiuk commented Jan 20, 2023

Uh oh!

Taragolis commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Jan 20, 2023

Uh oh!

Taragolis commented Jan 20, 2023

Uh oh!

potiuk commented Jan 20, 2023

Uh oh!

Taragolis commented Jan 21, 2023

Uh oh!

Taragolis commented Jan 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Taragolis commented Jan 20, 2023 •

edited

Loading