You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/09/23 19:39:43 UTC
[GitHub] [airflow] XD-DENG opened a new pull request, #26641: Clearer code for PodGenerator.deserialize_model_file
XD-DENG opened a new pull request, #26641:
URL: https://github.com/apache/airflow/pull/26641
Especially for how it handles non-existent file.
When the file path received doesn't exist, the current way is to use yaml.safe_load() to process it, and it returns the path as a string.
Then this string is passed to deserialize_model_dict() and results in an empty object. Passing 'None' to deserialize_model_dict() will do the same, but the code will become clearer, and less misleading.
Meanwhile, when the model file path received doesn't exist, there should be a warning in the log.
(This change shouldn't cause any behaviour change)
<!--
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/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.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
[GitHub] [airflow] XD-DENG commented on a diff in pull request #26641: Clearer code for PodGenerator.deserialize_model_file
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on code in PR #26641:
URL: https://github.com/apache/airflow/pull/26641#discussion_r979504851
##########
airflow/kubernetes/pod_generator.py:
##########
@@ -412,16 +415,13 @@ def deserialize_model_file(path: str) -> k8s.V1Pod:
"""
:param path: Path to the file
:return: a kubernetes.client.models.V1Pod
-
- Unfortunately we need access to the private method
- ``_ApiClient__deserialize_model`` from the kubernetes client.
- This issue is tracked here; https://github.com/kubernetes-client/python/issues/977.
"""
if os.path.exists(path):
with open(path) as stream:
pod = yaml.safe_load(stream)
else:
- pod = yaml.safe_load(path)
+ pod = None
+ log.warning("Model file %s does not exist", path)
return PodGenerator.deserialize_model_dict(pod)
Review Comment:
Hi @uranusjr !
Sorry for having missed it. Addressed it at https://github.com/apache/airflow/pull/26641/commits/f61a7e0eeda900bf1879e70c17dc95dc4fe98dc6
Thanks!
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #26641: Clearer code for PodGenerator.deserialize_model_file
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26641:
URL: https://github.com/apache/airflow/pull/26641#discussion_r979478365
##########
airflow/kubernetes/pod_generator.py:
##########
@@ -412,16 +415,13 @@ def deserialize_model_file(path: str) -> k8s.V1Pod:
"""
:param path: Path to the file
:return: a kubernetes.client.models.V1Pod
-
- Unfortunately we need access to the private method
- ``_ApiClient__deserialize_model`` from the kubernetes client.
- This issue is tracked here; https://github.com/kubernetes-client/python/issues/977.
"""
if os.path.exists(path):
with open(path) as stream:
pod = yaml.safe_load(stream)
else:
- pod = yaml.safe_load(path)
+ pod = None
+ log.warning("Model file %s does not exist", path)
return PodGenerator.deserialize_model_dict(pod)
Review Comment:
Need to change type annotation of this method as well.
--
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
[GitHub] [airflow] XD-DENG commented on pull request #26641: Clearer code for PodGenerator.deserialize_model_file
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on PR #26641:
URL: https://github.com/apache/airflow/pull/26641#issuecomment-1257287180
Hi @ashb , adding you as reviewer for this since you reviewed & co-authored the original change.
Pls let me know if this is making sense to you or additional info needed. Thanks!
--
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
[GitHub] [airflow] XD-DENG commented on a diff in pull request #26641: Clearer code for PodGenerator.deserialize_model_file
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on code in PR #26641:
URL: https://github.com/apache/airflow/pull/26641#discussion_r979013539
##########
airflow/kubernetes/pod_generator.py:
##########
@@ -412,16 +415,13 @@ def deserialize_model_file(path: str) -> k8s.V1Pod:
"""
:param path: Path to the file
:return: a kubernetes.client.models.V1Pod
-
- Unfortunately we need access to the private method
Review Comment:
This doctoring was missed to be moved during earlier refactoring https://github.com/apache/airflow/pull/10756
--
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
[GitHub] [airflow] XD-DENG commented on a diff in pull request #26641: Clearer code for PodGenerator.deserialize_model_file
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on code in PR #26641:
URL: https://github.com/apache/airflow/pull/26641#discussion_r979020707
##########
tests/kubernetes/test_pod_generator.py:
##########
@@ -713,10 +713,20 @@ def test_reconcile_specs_init_containers(self):
assert res.init_containers == base_spec.init_containers + client_spec.init_containers
def test_deserialize_model_file(self):
- path = sys.path[0] + '/tests/kubernetes/pod.yaml'
- result = PodGenerator.deserialize_model_file(path)
- sanitized_res = self.k8s_client.sanitize_for_serialization(result)
- assert sanitized_res == self.deserialize_result
+ with self.assertLogs(level='WARNING') as capture_logs:
+ path = sys.path[0] + '/tests/kubernetes/pod.yaml'
+ result = PodGenerator.deserialize_model_file(path)
+ sanitized_res = self.k8s_client.sanitize_for_serialization(result)
+ assert sanitized_res == self.deserialize_result
+ assert len(capture_logs.records) == 0
Review Comment:
No change is made to this test case itself, other than checking if the code dumped any warning log
--
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
[GitHub] [airflow] XD-DENG merged pull request #26641: Clearer code for PodGenerator.deserialize_model_file
Posted by GitBox <gi...@apache.org>.
XD-DENG merged PR #26641:
URL: https://github.com/apache/airflow/pull/26641
--
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
[GitHub] [airflow] XD-DENG commented on a diff in pull request #26641: Clearer code for PodGenerator.deserialize_model_file
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on code in PR #26641:
URL: https://github.com/apache/airflow/pull/26641#discussion_r979020707
##########
tests/kubernetes/test_pod_generator.py:
##########
@@ -713,10 +713,20 @@ def test_reconcile_specs_init_containers(self):
assert res.init_containers == base_spec.init_containers + client_spec.init_containers
def test_deserialize_model_file(self):
- path = sys.path[0] + '/tests/kubernetes/pod.yaml'
- result = PodGenerator.deserialize_model_file(path)
- sanitized_res = self.k8s_client.sanitize_for_serialization(result)
- assert sanitized_res == self.deserialize_result
+ with self.assertLogs(level='WARNING') as capture_logs:
+ path = sys.path[0] + '/tests/kubernetes/pod.yaml'
+ result = PodGenerator.deserialize_model_file(path)
+ sanitized_res = self.k8s_client.sanitize_for_serialization(result)
+ assert sanitized_res == self.deserialize_result
+ assert len(capture_logs.records) == 0
Review Comment:
No change is made to this test case itself, other than checking if the code dumped any warning log
--
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