Made api's compatible with python3.7+ #11259
Conversation
bharatjetti
requested review from
yeqingli,
rachellj218 and
lehougoogle
as code owners
laxmareddyp
requested review from
Chen-Zhe
and removed request for
rachellj218
|
Hi @yeqingli, @lehougoogle, @Chen-Zhe, could you please review the PR. Thank You |
laxmareddyp
added
the
ready to pull
laxmareddyp
removed
the
ready to pull
| @@ -33,7 +34,7 @@ class BaseTwoTowerUpliftNetwork(tf_keras.layers.Layer, metaclass=abc.ABCMeta): | |||
| def call( | |||
| self, | |||
| inputs: types.DictOfTensors, | |||
| training: bool | None = None, | |||
| mask: tf.Tensor | None = None, | |||
| training: Union[bool, None] = None, | |||
There was a problem hiding this comment.
I think Union[something | None] is equivalent to Optional[something]. Could you update here and all the other places if applicable.
official/vision/modeling/factory.py
Outdated
| @@ -263,8 +263,8 @@ def build_retinanet( | |||
| l2_regularizer: Optional[tf_keras.regularizers.Regularizer] = None, | |||
| backbone: Optional[tf_keras.Model] = None, | |||
| decoder: Optional[tf_keras.Model] = None, | |||
| num_anchors_per_location: int | dict[str, int] | None = None, | |||
| anchor_boxes: Mapping[str, tf.Tensor] | None = None, | |||
| num_anchors_per_location: Union[int, Dict[str, int], None] = None, | |||
There was a problem hiding this comment.
Can this Union[int, Dict[str, int], None] be Optional[Union[int, Dict[str, int]]]
| export_dir: str, | ||
| checkpoint_path: Optional[str] = None, |
There was a problem hiding this comment.
Can you keep the original order of arguments by moving checkpoint_path before export_dir?
There was a problem hiding this comment.
Updated the suggested changes.
| num_scales: int | None, | ||
| aspect_ratios: list[float] | None, | ||
| anchor_size: float | None, | ||
| min_level: Optional[int] = None, |
There was a problem hiding this comment.
Can you keep the original order of arguments?
There was a problem hiding this comment.
Updated the suggested changes.
|
|
||
| from official.core.config_definitions import base_config | ||
|
|
||
|
|
||
| @dataclasses.dataclass(kw_only=True) | ||
| @dataclasses.dataclass |
There was a problem hiding this comment.
Is there a specific reason to remove kw_only=True?
There was a problem hiding this comment.
To keep the code more flexible, simple by allowing positional arguments to define attributes for SlicedMetricConfig.
There was a problem hiding this comment.
Do you have a use case for this? Please do not change this if this change is not a must.
There was a problem hiding this comment.
I restored the kw_only=True as suggested.
Description
Reference github issue #11247 #11256
Type of change
For a new feature or function, please create an issue first to discuss it
with us before submitting a pull request.
Note: Please delete options that are not relevant.
Tests
Test Configuration:
Checklist