You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2024/12/01 23:41:12 UTC
[PR] Remove Pydanitc models introduced for AIP-44 [airflow]
potiuk opened a new pull request, #44552:
URL: https://github.com/apache/airflow/pull/44552
The Pudanic models have been used in a number of places still and we are using them also for context passing for PythonVirtualEnv and ExternaPythonOperator - this PR removes all the models and their usages.
Closes: #44436
# Please enter the commit message for your changes. Lines starting
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #44552:
URL: https://github.com/apache/airflow/pull/44552#discussion_r1874996568
##########
airflow/serialization/serialized_objects.py:
##########
@@ -784,7 +724,7 @@ def default_serialization(cls, strict, var) -> str:
return str(var)
@classmethod
- def deserialize(cls, encoded_var: Any, use_pydantic_models=False) -> Any:
+ def deserialize(cls, encoded_var: Any) -> Any:
Review Comment:
See the comment https://github.com/apache/airflow/pull/44552#issuecomment-2526280913
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "gopidesupavan (via GitHub)" <gi...@apache.org>.
gopidesupavan commented on code in PR #44552:
URL: https://github.com/apache/airflow/pull/44552#discussion_r1866207444
##########
airflow/serialization/serialized_objects.py:
##########
@@ -784,7 +724,7 @@ def default_serialization(cls, strict, var) -> str:
return str(var)
@classmethod
- def deserialize(cls, encoded_var: Any, use_pydantic_models=False) -> Any:
+ def deserialize(cls, encoded_var: Any) -> Any:
Review Comment:
The `use_pydantic_models` used in python operator https://github.com/apache/airflow/blob/main/providers/src/airflow/providers/standard/utils/python_virtualenv_script.jinja2#L81, might be changes required in here aswell?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #44552:
URL: https://github.com/apache/airflow/pull/44552#issuecomment-2512152947
happy we're removing `use_pydantic_models`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #44552:
URL: https://github.com/apache/airflow/pull/44552#issuecomment-2513304981
> happy we're removing use_pydantic_models
Yeah ... I had no time to look at the failures today ... but tomorrow 🤞 is the day :). I just feel that there will be some left-overs with the context and some TODOs to fix - it's worth to do it anyway :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #44552:
URL: https://github.com/apache/airflow/pull/44552#discussion_r1874984807
##########
airflow/serialization/serialized_objects.py:
##########
@@ -784,7 +724,7 @@ def default_serialization(cls, strict, var) -> str:
return str(var)
@classmethod
- def deserialize(cls, encoded_var: Any, use_pydantic_models=False) -> Any:
+ def deserialize(cls, encoded_var: Any) -> Any:
Review Comment:
One of the reasons I was not in a super-hurry with this one.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #44552:
URL: https://github.com/apache/airflow/pull/44552#issuecomment-2510318588
BTW. This is the last one in the #44436 series !
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #44552:
URL: https://github.com/apache/airflow/pull/44552
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #44552:
URL: https://github.com/apache/airflow/pull/44552#discussion_r1874984635
##########
airflow/serialization/serialized_objects.py:
##########
@@ -784,7 +724,7 @@ def default_serialization(cls, strict, var) -> str:
return str(var)
@classmethod
- def deserialize(cls, encoded_var: Any, use_pydantic_models=False) -> Any:
+ def deserialize(cls, encoded_var: Any) -> Any:
Review Comment:
Yes. That's why I think **some** TODOs will stay after this one as we are currently using Context Pydantic version to pass to PythonOperator - this one will have to be replaced with whatever Context serialization TaskSDK has (cc: @ashb @kaxil -> I have not looked at this yet, but since we are using HTTPs to send the context to SDK and Pydantic classes for Task SDK under-the hood, so I guess we will be able to re-use what's there.
For now I think I will just have to copy privately the Pydantic classes needed to the Python Operator, to make it possible to use standard operator, but we will eventually have to do it "better" - lilkely after this PR is merged.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #44552:
URL: https://github.com/apache/airflow/pull/44552#issuecomment-2539354385
Reopen with full-tests-needed
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #44552:
URL: https://github.com/apache/airflow/pull/44552#issuecomment-2530790614
> also current Fast API implementation uses DagTagPydantic as serialization layer. I am not sure if we can do it differently @pierrejeambrun but that sounds like there should be an easy way to do so?
We just need to copy into the `api_fastapi/core_api/datamodels` the `DagTagPydantic`. (and rename it into `DagTagResponse`, those are specific RestAPI serializers). So we can completely delete Pydantic model from AIP-44).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #44552:
URL: https://github.com/apache/airflow/pull/44552#discussion_r1874999017
##########
airflow/serialization/serialized_objects.py:
##########
@@ -784,7 +724,7 @@ def default_serialization(cls, strict, var) -> str:
return str(var)
@classmethod
- def deserialize(cls, encoded_var: Any, use_pydantic_models=False) -> Any:
+ def deserialize(cls, encoded_var: Any) -> Any:
Review Comment:
We haven't tackled context yet, but these models can be removed and we'll add back the ones we need in the execution SDK (some will be very different, some similar) so merge when ever you are happy with this
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #44552:
URL: https://github.com/apache/airflow/pull/44552#issuecomment-2510307818
It's not likely this one will be green, but it's likely it will show which parts have to be fixed because they used implicitly Pydantic Models synchronisation.
But generally speaking it is ready to take a first pass.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #44552: Remove Pydanitc models introduced for AIP-44
URL: https://github.com/apache/airflow/pull/44552
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #44552:
URL: https://github.com/apache/airflow/pull/44552#issuecomment-2539765453
Ok. Looks like Pydantic classes removal is ready to merge - any last pass? I moved the DagTag to fast_api models, and added error messages/conditional tests/ left TODOs to bring `use_airflow_context` when we have serializable context for AIP-72.
If anyone want to have last pass -> feel free.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #44552:
URL: https://github.com/apache/airflow/pull/44552#issuecomment-2526280913
Speaking of which. I think we have to agree on Pydantic strategy here.
There are two reasons why this one is "red"
* as discussed before - we are passing Context in PythonVirtualenv operator via Cwhen `use_airflow_context=True` - which is likely not good idea for Standard provider.
So far it was really a "future" feature - only working with Airflow 3 and in Airflow 2 it was only working with AIP-44 enabled (which was never a production option, except a few crazy people (cc: @jscheffl ;). So we could possibly even drop it. Or we can likely serialize it using the same mechanism serialization will happen for TaskSDK (@kaxil @ashb)? What would it be? I looked at the datamodels in task_sdk and have not seen context autogenerated yet (did I look in the right place?) maybe it's somewhere else.
So far context was serializable with assumption that AIP-44 is enabled and the "ORM" objects were serialized to their Pydantic counterparts - the whole contex serialization was added here https://github.com/apache/airflow/pull/38567 by @dstandish and it assumed all the ORM models that were part of the context were serializable (this will stop being true once we remove the `*Pydantic classes'
What plans do you have for for context serialization ?
* also current Fast API implementation uses DagTagPydantic as serialization layer. I am not sure if we can do it differently @pierrejeambrun but that sounds like there should be an easy way to do so?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Pydanitc models introduced for AIP-44 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #44552:
URL: https://github.com/apache/airflow/pull/44552#issuecomment-2530919845
So from those two responses:
> We haven't tackled context yet, but these models can be removed and we'll add back the ones we need in the execution SDK (some will be very different, some similar) so merge when ever you are happy with this
> We just need to copy into the `api_fastapi/core_api/datamodels` the `DagTagPydantic`. (and rename it into `DagTagResponse`, those are specific RestAPI serializers). The API has it own serializers, but this one was common so I re-used it, but I'm not sure it was even a good idea in the first place. (Now that all those are going away there's no hesitation anymore 😀)
Thanks for those answers!
Let me try to complete this one then. I will leave the "use_airflow_context" as a feature of Python Virtualenv that only (will) work in Airflow 3 and leave to DO to add context propagation once we have it done for AIP-72.
And we should be able to close this one.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org