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.

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

Send context using in venv operator #41363

Merged
merged 51 commits into from
Aug 14, 2024
Merged

Conversation

phi-friday
Copy link
Contributor

origin: #41039
related: #41362


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@eladkal eladkal added the full tests needed We need to run full set of tests for this PR to merge label Aug 9, 2024
@eladkal eladkal closed this Aug 9, 2024
@eladkal eladkal reopened this Aug 9, 2024
@eladkal
Copy link
Contributor

eladkal commented Aug 9, 2024

something is wrong as it shows 0 files changed
ah you need to wait till the revert PR is merged. otherwise the diff is nothing

@phi-friday
Copy link
Contributor Author

something is wrong as it shows 0 files changed ah you need to wait till the revert PR is merged. otherwise the diff is nothing

I'm not sure what to do as I've never seen this before.
If #41362 is merged, should I just rebase it?

@phi-friday
Copy link
Contributor Author

phi-friday commented Aug 9, 2024

Based on the error message given by #41362, I checked and it only works with pydantic>=2.
I think we need to either modify the function _pydantic_model_dump or set use_pydantic_models to False.
We could also make this only available for pydantic>=2.

@eladkal eladkal requested a review from shahar1 August 9, 2024 17:49
@phi-friday
Copy link
Contributor Author

phi-friday commented Aug 9, 2024

I don't know if this is allowed.
Modifying the model as follows did not cause the problem,
even if it was pydantic<2 or none (I checked in local, only pydantic test).

in airflow.utils.pydantic

if is_pydantic_2_installed():
    from pydantic import BaseModel, ConfigDict, PlainSerializer, PlainValidator, ValidationInfo
else:

    class BaseModel:  # type: ignore[no-redef]  # noqa: D101
        def __init__(self, *args, **kwargs):
            self.__dict__.update(kwargs)

        @classmethod
        def model_validate(cls, value: object):
            args = {name: getattr(value, name, None) for name in cls.__annotations__}
            return cls(**args)

        def model_dump(self, *args, **kwargs):
            from airflow.serialization.serialized_objects import BaseSerialization

            return {
                name: BaseSerialization.serialize(getattr(self, name, None), use_pydantic_models=True)
                for name in self.__annotations__
            }

@potiuk
Copy link
Member

potiuk commented Aug 11, 2024

Ah yes. This is because serialization uses Pydantic currently to serialize DB models that are part of the context and that requires Pydantic v2 to be installed. I'd say we should make this feature only available if Pydantic2 is installed (with approproate documentation and error messages) - and skip the tests if Pydantic 2 is not installed.

@eladkal eladkal added this to the Airflow 3.0.0 milestone Aug 12, 2024
@eladkal eladkal added the type:improvement Changelog: Improvements label Aug 12, 2024
@phi-friday
Copy link
Contributor Author

if use_pydantic_models and not _ENABLE_AIP_44:

If AIP44 is not enabled, an error is thrown.
So I made it so that an error is thrown if pydantic<2 or AIP44 is not enabled.
And I added test cases for each of them.

@potiuk potiuk merged commit 645c3cd into apache:main Aug 14, 2024
80 checks passed
@phi-friday phi-friday deleted the feat-venv-context branch August 14, 2024 04:00
Artuz37 pushed a commit to Artuz37/airflow that referenced this pull request Aug 19, 2024
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants