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 2021/12/03 16:10:53 UTC
[GitHub] [airflow] ashb opened a new pull request #20019: Change ParamsDict to a MutableMapping subclass
ashb opened a new pull request #20019:
URL: https://github.com/apache/airflow/pull/20019
This is a little more work, and while it has no change in behaviour it
does make Mypy happier -- previously it was complaining about update's
signature not matching dict _or_ MutableMapping's
Part of #19891, but since this was a bigger change than just adding/changing some types it is its own PR for easier review.
--
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] potiuk commented on a change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r762080010
##########
File path: airflow/models/param.py
##########
@@ -99,16 +99,17 @@ def has_value(self) -> bool:
return not isinstance(self.value, NoValueSentinel)
-class ParamsDict(dict):
+class ParamsDict(MutableMapping):
"""
Class to hold all params for dags or tasks. All the keys are strictly string and values
are converted into Param's object if they are not already. This class is to replace param's
dictionary implicitly and ideally not needed to be used directly.
"""
+ __slots__ = ['__dict__', 'supresss_exception']
Review comment:
> Without a `__dict__` variable, instances cannot be assigned new variables not listed in the `__slots__` definition. Attempts to assign to an unlisted variable name raises AttributeError. If dynamic assignment of new variables is desired, then add `'__dict__'` to the sequence of strings in the `__slots__` declaration.
--
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] ashb commented on a change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r763729890
##########
File path: tests/models/test_param.py
##########
@@ -167,18 +167,28 @@ def test_params_dict(self):
assert pd3.dump() == {'key': 'value'}
# Validate the ParamsDict
- pd.validate()
+ plain_dict = pd.validate()
+ assert type(plain_dict) == dict
pd2.validate()
pd3.validate()
# Update the ParamsDict
- with pytest.raises(ValueError):
+ with pytest.raises(ValueError, match=r'Invalid input for param key: 1 is not'):
Review comment:
The rest of the error message depends on the format of the exception thrown by the JSONSchema library we use so I didn't want to be too specific here.
--
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] potiuk commented on a change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r762080010
##########
File path: airflow/models/param.py
##########
@@ -99,16 +99,17 @@ def has_value(self) -> bool:
return not isinstance(self.value, NoValueSentinel)
-class ParamsDict(dict):
+class ParamsDict(MutableMapping):
"""
Class to hold all params for dags or tasks. All the keys are strictly string and values
are converted into Param's object if they are not already. This class is to replace param's
dictionary implicitly and ideally not needed to be used directly.
"""
+ __slots__ = ['__dict__', 'supresss_exception']
Review comment:
> Without a __dict__ variable, instances cannot be assigned new variables not listed in the __slots__ definition. Attempts to assign to an unlisted variable name raises AttributeError. If dynamic assignment of new variables is desired, then add '__dict__' to the sequence of strings in the __slots__ declaration.
##########
File path: airflow/models/param.py
##########
@@ -99,16 +99,17 @@ def has_value(self) -> bool:
return not isinstance(self.value, NoValueSentinel)
-class ParamsDict(dict):
+class ParamsDict(MutableMapping):
"""
Class to hold all params for dags or tasks. All the keys are strictly string and values
are converted into Param's object if they are not already. This class is to replace param's
dictionary implicitly and ideally not needed to be used directly.
"""
+ __slots__ = ['__dict__', 'supresss_exception']
Review comment:
TIL
--
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] ashb commented on a change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r762427969
##########
File path: airflow/models/param.py
##########
@@ -14,7 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from typing import Any, Dict, Optional
+from typing import Any, Dict, ItemsView, MutableMapping, Optional, ValuesView
Review comment:
3.9 I think (or maybe 3.8)
--
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] potiuk commented on a change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r762080010
##########
File path: airflow/models/param.py
##########
@@ -99,16 +99,17 @@ def has_value(self) -> bool:
return not isinstance(self.value, NoValueSentinel)
-class ParamsDict(dict):
+class ParamsDict(MutableMapping):
"""
Class to hold all params for dags or tasks. All the keys are strictly string and values
are converted into Param's object if they are not already. This class is to replace param's
dictionary implicitly and ideally not needed to be used directly.
"""
+ __slots__ = ['__dict__', 'supresss_exception']
Review comment:
```
Without a __dict__ variable, instances cannot be assigned new variables not listed in the __slots__ definition. Attempts to assign to an unlisted variable name raises AttributeError. If dynamic assignment of new variables is desired, then add '__dict__' to the sequence of strings in the __slots__ declaration.
```
--
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 change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r762427786
##########
File path: airflow/models/param.py
##########
@@ -99,16 +99,17 @@ def has_value(self) -> bool:
return not isinstance(self.value, NoValueSentinel)
-class ParamsDict(dict):
+class ParamsDict(MutableMapping):
Review comment:
```suggestion
class ParamsDict(MutableMapping[str, Any]):
```
This would allow annotating return values of `keys()` and `items()` (maybe some more).
--
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 change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r762427786
##########
File path: airflow/models/param.py
##########
@@ -99,16 +99,17 @@ def has_value(self) -> bool:
return not isinstance(self.value, NoValueSentinel)
-class ParamsDict(dict):
+class ParamsDict(MutableMapping):
Review comment:
```suggestion
class ParamsDict(MutableMapping[str, Any]):
```
This would allow annotating `keys` and `items`.
--
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 change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r762427724
##########
File path: airflow/models/param.py
##########
@@ -14,7 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from typing import Any, Dict, Optional
+from typing import Any, Dict, ItemsView, MutableMapping, Optional, ValuesView
Review comment:
Inheriting from typing is required for generic support, I forgot until what version. Inheriting from `collections.abc` will be viable at some point, but we’re probably not there yet.
--
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 change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r763639901
##########
File path: airflow/models/param.py
##########
@@ -154,30 +170,34 @@ def __getitem__(self, key: str) -> Any:
:param key: The key to fetch
:type key: str
"""
- param = super().__getitem__(key)
+ param = self.__dict[key]
return param.resolve(suppress_exception=self.suppress_exception)
+ def get_param(self, key: str) -> Param:
+ """Get the internal :class:`.Param` object for this key"""
+ return self.__dict[key]
+
+ def items(self):
+ return ItemsView(self.__dict)
+
+ def values(self):
+ return ValuesView(self.__dict)
+
+ def update(self, *args, **kwargs) -> None:
+
+ if len(args) == 1 and isinstance(args[0], ParamsDict):
Review comment:
```suggestion
if len(args) == 1 and not kwargs and isinstance(args[0], ParamsDict):
```
If this is called with an invalid argument combination like `params.update({"b": 1}, c=2)`, we want to let the `super()` call catch it and emit an error.
##########
File path: tests/models/test_param.py
##########
@@ -167,18 +167,28 @@ def test_params_dict(self):
assert pd3.dump() == {'key': 'value'}
# Validate the ParamsDict
- pd.validate()
+ plain_dict = pd.validate()
+ assert type(plain_dict) == dict
pd2.validate()
pd3.validate()
# Update the ParamsDict
- with pytest.raises(ValueError):
+ with pytest.raises(ValueError, match=r'Invalid input for param key: 1 is not'):
Review comment:
I assume this is not the complete error message? (If it is, we should change it…) It’s probably a good idea to add an extra assert to make sure the full error message is as expected.
##########
File path: airflow/models/param.py
##########
@@ -121,9 +122,21 @@ def __init__(self, dict_obj: Optional[Dict] = None, suppress_exception: bool = F
params_dict[k] = Param(v)
else:
params_dict[k] = v
- super().__init__(params_dict)
+ self.__dict = params_dict
self.suppress_exception = suppress_exception
+ def __contains__(self, o: object) -> bool:
+ return o in self.__dict
+
+ def __len__(self) -> int:
+ return len(self.__dict)
+
+ def __delitem__(self, v: str) -> None:
+ del self.__dict[v]
+
+ def __iter__(self):
+ return self.__dict.__iter__()
Review comment:
```suggestion
return iter(self.__dict)
```
For consistency—we didn’t use the dict’s magic methods for all other magic method implementations.
--
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 change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r762072353
##########
File path: airflow/models/param.py
##########
@@ -99,16 +99,17 @@ def has_value(self) -> bool:
return not isinstance(self.value, NoValueSentinel)
-class ParamsDict(dict):
+class ParamsDict(MutableMapping):
"""
Class to hold all params for dags or tasks. All the keys are strictly string and values
are converted into Param's object if they are not already. This class is to replace param's
dictionary implicitly and ideally not needed to be used directly.
"""
+ __slots__ = ['__dict__', 'supresss_exception']
Review comment:
```suggestion
__slots__ = ['__dict', 'supresss_exception']
```
I think?
--
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] potiuk commented on a change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r762081224
##########
File path: airflow/models/param.py
##########
@@ -99,16 +99,17 @@ def has_value(self) -> bool:
return not isinstance(self.value, NoValueSentinel)
-class ParamsDict(dict):
+class ParamsDict(MutableMapping):
"""
Class to hold all params for dags or tasks. All the keys are strictly string and values
are converted into Param's object if they are not already. This class is to replace param's
dictionary implicitly and ideally not needed to be used directly.
"""
+ __slots__ = ['__dict__', 'supresss_exception']
Review comment:
https://docs.python.org/3/reference/datamodel.html#notes-on-using-slots
--
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 change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r762427786
##########
File path: airflow/models/param.py
##########
@@ -99,16 +99,17 @@ def has_value(self) -> bool:
return not isinstance(self.value, NoValueSentinel)
-class ParamsDict(dict):
+class ParamsDict(MutableMapping):
Review comment:
```suggestion
class ParamsDict(MutableMapping[str, Any]):
```
--
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 change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r763731225
##########
File path: tests/models/test_param.py
##########
@@ -167,18 +167,28 @@ def test_params_dict(self):
assert pd3.dump() == {'key': 'value'}
# Validate the ParamsDict
- pd.validate()
+ plain_dict = pd.validate()
+ assert type(plain_dict) == dict
pd2.validate()
pd3.validate()
# Update the ParamsDict
- with pytest.raises(ValueError):
+ with pytest.raises(ValueError, match=r'Invalid input for param key: 1 is not'):
Review comment:
Makes sense (especially since I _just_ fixed a PR yesterday because MarkupSafe changed its exception message format).
--
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] ashb commented on a change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r762081845
##########
File path: airflow/models/param.py
##########
@@ -99,16 +99,17 @@ def has_value(self) -> bool:
return not isinstance(self.value, NoValueSentinel)
-class ParamsDict(dict):
+class ParamsDict(MutableMapping):
"""
Class to hold all params for dags or tasks. All the keys are strictly string and values
are converted into Param's object if they are not already. This class is to replace param's
dictionary implicitly and ideally not needed to be used directly.
"""
+ __slots__ = ['__dict__', 'supresss_exception']
Review comment:
Ohhh, by making `__dict__` a slot I made it the same as having no slots at all :D
--
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] github-actions[bot] commented on pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#issuecomment-985743705
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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] ashb commented on a change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r762073252
##########
File path: airflow/models/param.py
##########
@@ -99,16 +99,17 @@ def has_value(self) -> bool:
return not isinstance(self.value, NoValueSentinel)
-class ParamsDict(dict):
+class ParamsDict(MutableMapping):
"""
Class to hold all params for dags or tasks. All the keys are strictly string and values
are converted into Param's object if they are not already. This class is to replace param's
dictionary implicitly and ideally not needed to be used directly.
"""
+ __slots__ = ['__dict__', 'supresss_exception']
Review comment:
Hmmm, yes. I wonder how that even worked, since it's a string so probably needs to be `_ParamsDict__dict`:thinking:
--
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] jedcunningham commented on a change in pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #20019:
URL: https://github.com/apache/airflow/pull/20019#discussion_r762261951
##########
File path: airflow/models/param.py
##########
@@ -14,7 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from typing import Any, Dict, Optional
+from typing import Any, Dict, ItemsView, MutableMapping, Optional, ValuesView
Review comment:
Should we be getting `MutableMapping` from `collections.abc` instead? Feels a little weird to inherit from something out of typing.
--
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] ashb merged pull request #20019: Change ParamsDict to a MutableMapping subclass
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #20019:
URL: https://github.com/apache/airflow/pull/20019
--
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