Skip to content
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.

Sign up for GitHub

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

Jump to bottom

openlineage: add ClassVar to Airflow facets. #36084

Merged
merged 1 commit into from
Dec 6, 2023
Merged

openlineage: add ClassVar to Airflow facets. #36084

merged 1 commit into from
Dec 6, 2023

Conversation

JDarDagran
Copy link
Contributor

Recent OpenLineage release introduced a change that breaks mypy in main. This PR fixes it.

@JDarDagran
Copy link
Contributor Author

JDarDagran commented Dec 6, 2023 •
edited
Loading

I'm not sure why mypy fails now - it looks the opposite to what happens in main.
How to check what versions of packages are installed in the CI env?

I ran breeze ci-image --upgrade-to-newer-dependencies locally after which got:
Run mypy for providers.............................................................Passed

Taragolis
Taragolis reviewed Dec 6, 2023
View reviewed changes
Copy link
Contributor

@Taragolis Taragolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't / shouldn't change parent class variable type: https://docs.python.org/3/library/typing.html#typing.ClassVar

That is the reason why mypy unhappy

airflow/providers/openlineage/plugins/facets.py Outdated
@@ -28,7 +30,7 @@ class AirflowMappedTaskRunFacet(BaseFacet):
mapIndex: int
operatorClass: str

_additional_skip_redact: list[str] = ["operatorClass"]
_additional_skip_redact: ClassVar[list[str]] = ["operatorClass"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_additional_skip_redact: ClassVar[list[str]] = ["operatorClass"]
_additional_skip_redact = ["operatorClass"]

airflow/providers/openlineage/plugins/facets.py Outdated
@@ -63,7 +65,7 @@ class UnknownOperatorInstance(RedactMixin):
properties: dict[str, object]
type: str = "operator"

_skip_redact: list[str] = ["name", "type"]
_skip_redact: ClassVar[list[str]] = ["name", "type"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_skip_redact: ClassVar[list[str]] = ["name", "type"]
_skip_redact = ["name", "type"]

@potiuk
Copy link
Member

potiuk commented Dec 6, 2023

I'm not sure why mypy fails now - it looks the opposite to what happens in main.
How to check what versions of packages are installed in the CI env?

If PR does not change dependencies (setup.py/generated dependencies.json) then dependencies are taken from. Main constraints file. Those are the very same dependencies you have when you just build the image without --upgrade-to-newer-dependencies

@potiuk
Copy link
Member

potiuk commented Dec 6, 2023

@JDarDagran https://github.com/apache/airflow/tree/constraints-main

@JDarDagran
Copy link
Contributor Author

Okay, how about we remove annotations in provider now as suggested by @Taragolis?
Not sure if we ever want to add them back once contraints for openlineage-python are updated with pin to version higher than 1.5.0?

@potiuk
Copy link
Member

potiuk commented Dec 6, 2023

Longer explanation:

I'm not sure why mypy fails now - it looks the opposite to what happens in main.

Yes. This is entirely expected that your PR will have 'regular' constraints. Only the main build (also known as canary will attempt to upgrade dependencies to the newer ones.

For 'regular` PRs the PR are using 'frozen' constraints which are automatically updated by the latest main build that passed all tests. This way when there is a change in released packages) - such as this one - all the regular PRs that do not touch dependencies will run using 'good' versions of packages.

This is all as designed as expected - this way change like this only fails canary build and a handful of PRs that change dependencies, but all 'standard' PRs are unaffected.

So yes - your PR uses the 'previous' version of openlineage libs - so you have to make your fix in the way that it is backwards compatible - and works for both - whAt was before the change and after. This is precisely what our users will have - some of them will use previous version of open lineage and some use the new version.

If you want to make a change that only works for newer version (which I think is not the case here) then you also have to update requirements for open lineage provider (and add >= x.y.z for example).

This way your PR will be 'changing' dependencies which means that it will automatically use 'upgrade to newer'

This is all precisely as designed and expected :)

@JDarDagran
Copy link
Contributor Author

JDarDagran commented Dec 6, 2023 •
edited
Loading

@potiuk thanks for your thorough explanation.
After all it seems like I should bump the dependency version as the change in openlineage-python is correct, we just want to have all new versions of OL provider to be compatible with this change. Is this reasoning correct?

After discussion in Slack I decided to add type: ignore with explanation.

potiuk
potiuk reviewed Dec 6, 2023
View reviewed changes
airflow/providers/openlineage/plugins/facets.py Outdated
_additional_skip_redact: list[str] = ["operatorClass"]
# openlineage-python<=1.5.0 does not annotate this as ClassVar
# mypy may complain about overriding instance variable with class variable
_additional_skip_redact: ClassVar[list[str]] = ["operatorClass"] # type: ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_additional_skip_redact: ClassVar[list[str]] = ["operatorClass"] # type: ignore
_additional_skip_redact: ClassVar[list[str]] = ["operatorClass"] # type: ignore[misc]

potiuk
potiuk reviewed Dec 6, 2023
View reviewed changes
airflow/providers/openlineage/plugins/facets.py Outdated
_skip_redact: list[str] = ["name", "type"]
# openlineage-python<=1.5.0 does not annotate this as ClassVar
# mypy may complain about overriding instance variable with class variable
_skip_redact: ClassVar[list[str]] = ["name", "type"] # type: ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_skip_redact: ClassVar[list[str]] = ["name", "type"] # type: ignore
_skip_redact: ClassVar[list[str]] = ["name", "type"] # type: ignore[misc]

@potiuk
Copy link
Member

potiuk commented Dec 6, 2023

BTw. @Taragolis in slack mentioned that just removing the annotation might work ... so maybe ?

@JDarDagran
Remove ClassVar annotations.
f28ffc2 
Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>
@JDarDagran JDarDagran force-pushed the openlineage/fix-classvars branch from 1ce6231 to f28ffc2 Compare December 6, 2023 13:42
@JDarDagran
Copy link
Contributor Author

@potiuk @Taragolis yeah, in the end we all agreed on removing the annotation. Went for it

potiuk
potiuk approved these changes Dec 6, 2023
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D

@mobuchowski mobuchowski merged commit fba682b into apache:main Dec 6, 2023
49 checks passed
@eladkal eladkal mentioned this pull request Dec 7, 2023
Closed
50 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Taragolis Taragolis Taragolis approved these changes

@uranusjr uranusjr uranusjr approved these changes

@potiuk potiuk potiuk approved these changes

Assignees
No one assigned
Labels
area:providers provider:openlineage AIP-53
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JDarDagran @potiuk @uranusjr @Taragolis @mobuchowski