Re: [PR] Preserve all context keys during serialization [airflow]
amoghrajesh commented on code in PR #50446: URL: https://github.com/apache/airflow/pull/50446#discussion_r2085977788 ## providers/standard/tests/unit/standard/operators/test_python.py: ## @@ -1451,6 +1451,99 @@ def f( self.run_as_task(f, serializer=serializer, system_site_packages=False, requirements=None) [email protected]( +"requirements, system_site, want_airflow, want_pendulum", +[ +# nothing → just base keys +([], False, False, False), +# site-packages → base keys + pendulum keys +([], True, True, True), +# apache-airflow / no version constraint +(["apache-airflow"], False, True, True), +# specific version +(["apache-airflow==2.10.2"], False, True, True), +# minimum version +(["apache-airflow>=2.10"], False, True, True), +# pendulum / no version constraint +(["pendulum"], False, False, True), +# compatible release +(["pendulum~=2.1.0"], False, False, True), +# other package +(["foo==1.0.0"], False, False, False), +# with other package +(["apache-airflow", "foo"], False, True, True), +# full-line comment only +(["# comment"], False, False, False), +# inline comment after requirement +(["apache-airflow==2.10.2 # comment"], False, True, True), +# blank line + requirement +(["", "pendulum"], False, False, True), +# indented comment + requirement +([" # comment", "pendulum~=2.1.0"], False, False, True), +], +) +def test_iter_serializable_context_keys(self, requirements, system_site, want_airflow, want_pendulum): +def func(): +return "test_return_value" + +op = PythonVirtualenvOperator( +task_id="task", +python_callable=func, +requirements=requirements, +system_site_packages=system_site, +) +keys = set(op._iter_serializable_context_keys()) + +base_keys = set(op.BASE_SERIALIZABLE_CONTEXT_KEYS) +airflow_keys = set(op.AIRFLOW_SERIALIZABLE_CONTEXT_KEYS) +pendulum_keys = set(op.PENDULUM_SERIALIZABLE_CONTEXT_KEYS) + +# BASE keys always present +assert base_keys <= keys + +# AIRFLOW keys only when expected +if want_airflow: +assert airflow_keys <= keys, f"expected AIRFLOW keys for requirements: {requirements}" +else: +assert not (airflow_keys & keys), f"unexpected AIRFLOW keys for requirements: {requirements}" + +# PENDULUM keys only when expected +if want_pendulum: +assert pendulum_keys <= keys, f"expected PENDULUM keys for requirements: {requirements}" +else: +assert not (pendulum_keys & keys), f"unexpected PENDULUM keys for requirements: {requirements}" + [email protected]( +"invalid_requirement", +[ +# invalid version format +"pendulum==3..0", +# invalid operator (=< instead of <=) +"apache-airflow=<2.0", +# same invalid operator on pendulum +"pendulum=<3.0", +# totally malformed +"invalid requirement", Review Comment: We do not account for the case when more than one are passed. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Preserve all context keys during serialization [airflow]
boring-cyborg[bot] commented on PR #50446: URL: https://github.com/apache/airflow/pull/50446#issuecomment-2873486403 Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Preserve all context keys during serialization [airflow]
shahar1 merged PR #50446: URL: https://github.com/apache/airflow/pull/50446 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Preserve all context keys during serialization [airflow]
yannlambret commented on code in PR #50446:
URL: https://github.com/apache/airflow/pull/50446#discussion_r2083921471
##
providers/standard/src/airflow/providers/standard/operators/python.py:
##
@@ -848,10 +852,39 @@ def execute_callable(self):
def _iter_serializable_context_keys(self):
yield from self.BASE_SERIALIZABLE_CONTEXT_KEYS
-if self.system_site_packages or "apache-airflow" in self.requirements:
+
+found_airflow = found_pendulum = False
+
+if self.system_site_packages:
+# If we're using system packages, assume both are present
+found_airflow = found_pendulum = True
+else:
+for raw_str in self.requirements:
+line = raw_str.strip()
+# Skip blank lines and full‐line comments
+if not line or line.startswith("#"):
+continue
+
+# Strip off any inline comment
+# e.g. turn "foo==1.2.3 # comment" → "foo==1.2.3"
+req_str = re.sub(r"#.*$", "", line).strip()
+
+try:
+req = Requirement(req_str)
+except (InvalidRequirement, InvalidSpecifier, InvalidVersion)
as e:
+raise ValueError(f"Invalid requirement '{raw_str}': {e}")
from e
+
+name = req.name.lower()
Review Comment:
Good point, I'm changing 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Preserve all context keys during serialization [airflow]
potiuk commented on code in PR #50446:
URL: https://github.com/apache/airflow/pull/50446#discussion_r2083638124
##
providers/standard/src/airflow/providers/standard/operators/python.py:
##
@@ -848,10 +852,39 @@ def execute_callable(self):
def _iter_serializable_context_keys(self):
yield from self.BASE_SERIALIZABLE_CONTEXT_KEYS
-if self.system_site_packages or "apache-airflow" in self.requirements:
+
+found_airflow = found_pendulum = False
+
+if self.system_site_packages:
+# If we're using system packages, assume both are present
+found_airflow = found_pendulum = True
+else:
+for raw_str in self.requirements:
+line = raw_str.strip()
+# Skip blank lines and full‐line comments
+if not line or line.startswith("#"):
+continue
+
+# Strip off any inline comment
+# e.g. turn "foo==1.2.3 # comment" → "foo==1.2.3"
+req_str = re.sub(r"#.*$", "", line).strip()
+
+try:
+req = Requirement(req_str)
+except (InvalidRequirement, InvalidSpecifier, InvalidVersion)
as e:
+raise ValueError(f"Invalid requirement '{raw_str}': {e}")
from e
+
+name = req.name.lower()
Review Comment:
NIT. I think Requirement already normalizes package name - which covers
replacement of ".", "_" with "-" and lowercasing the name:
https://packaging.python.org/en/latest/specifications/name-normalization/
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
