Skip to content

Conversation

@guoqingbao
Copy link

This issue is related to #2172 and #2180

@yan12125
Copy link
Contributor

yan12125 commented Aug 7, 2023

Thank you very much for this PR!

There are some pylint errors:

************* Module tf2onnx.convert tf2onnx/convert.py:479:53: C0303: Trailing whitespace (trailing-whitespace) tf2onnx/convert.py:482:54: C0303: Trailing whitespace (trailing-whitespace) tf2onnx/convert.py:484:0: C0303: Trailing whitespace (trailing-whitespace) ************* Module tf2onnx.tf_loader tf2onnx/tf_loader.py:574:57: C0303: Trailing whitespace (trailing-whitespace) tf2onnx/tf_loader.py:577:58: C0303: Trailing whitespace (trailing-whitespace) tf2onnx/tf_loader.py:577:29: W0212: Access to a protected member _captures of a client class (protected-access) 

trailing-whitespace is intuitive. For protected-access, most likely # pylint: disable=protected-access should be added to the line using _captures attribute, like what original codes do.

@guoqingbao
Copy link
Author

Thank you very much for this PR!

There are some pylint errors:

************* Module tf2onnx.convert tf2onnx/convert.py:479:53: C0303: Trailing whitespace (trailing-whitespace) tf2onnx/convert.py:482:54: C0303: Trailing whitespace (trailing-whitespace) tf2onnx/convert.py:484:0: C0303: Trailing whitespace (trailing-whitespace) ************* Module tf2onnx.tf_loader tf2onnx/tf_loader.py:574:57: C0303: Trailing whitespace (trailing-whitespace) tf2onnx/tf_loader.py:577:58: C0303: Trailing whitespace (trailing-whitespace) tf2onnx/tf_loader.py:577:29: W0212: Access to a protected member _captures of a client class (protected-access) 

trailing-whitespace is intuitive. For protected-access, most likely # pylint: disable=protected-access should be added to the line using _captures attribute, like what original codes do.

I have fixed the pylint warning, please refer to the latest commit.

@yan12125
Copy link
Contributor

yan12125 commented Aug 7, 2023

Thank you. There are still pylint errors about trailing whitespaces. Could you remove them all?

Signed-off-by: Guoqing Bao <guoqing.bao@enflame-tech.com>
Signed-off-by: Guoqing Bao <guoqing.bao@enflame-tech.com>
Signed-off-by: Guoqing Bao <guoqing.bao@enflame-tech.com>
@guoqingbao
Copy link
Author

Thank you. There are still pylint errors about trailing whitespaces. Could you remove them all?

I have removed the trailing whitespace, could you help merge this request?

@yan12125
Copy link
Contributor

Thank you for the update. I don't have the permission to merge pull requests in this repo. I noticed a maintainer imported changes here to a new pulll request #2216, so this pull request is probably in scope.

@fatcat-z
Copy link
Collaborator

Thanks for your contributions! This will be included in another PR which aims to support the latest version of TensorFlow. Please feel free to close this.

@guoqingbao
Copy link
Author

Thanks for your contributions! This will be included in another PR which aims to support the latest version of TensorFlow. Please feel free to close this.

I saw there is a PR that changed "_captures" to "captures" for the latest TensorFlow, but that will not be compatible with the lower version of TensorFlow, right?

@fatcat-z
Copy link
Collaborator

Thanks for your contributions! This will be included in another PR which aims to support the latest version of TensorFlow. Please feel free to close this.

I saw there is a PR that changed "_captures" to "captures" for the latest TensorFlow, but that will not be compatible with the lower version of TensorFlow, right?

The code in #2216 supports lower versions of TensorFlow as well. The logic is almost as same as yours but your code checks "captures" while that code checks "_captures".

@guoqingbao
Copy link
Author

Thanks for your contributions! This will be included in another PR which aims to support the latest version of TensorFlow. Please feel free to close this.

I saw there is a PR that changed "_captures" to "captures" for the latest TensorFlow, but that will not be compatible with the lower version of TensorFlow, right?

The code in #2216 supports lower versions of TensorFlow as well. The logic is almost as same as yours but your code checks "captures" while that code checks "_captures".

That's great! I come out with this based on the earlier revision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants