Skip to content

Fix validation of dynamic axes names #23974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

MaximKalininMS
Copy link
Contributor

Existing code adds two enumerators to the set instead of forming their union.

Existing code adds two enumerators to the set instead of forming their union.
@pytorchbot pytorchbot added the module: onnx Related to torch.onnx label Aug 7, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ezyang
Copy link
Contributor

ezyang commented Aug 9, 2019

It would be really nice if there was a test, but this is the second time I saw this exact patch in the PR queue, so let's go and merge it first...

@MaximKalininMS
Copy link
Contributor Author

@ezyang I added a test. Any idea why it failed in "ci/circleci: caffe2_onnx_py2_gcc5_ubuntu16_04_test"?

@ezyang
Copy link
Contributor

ezyang commented Aug 10, 2019

Seems like the new test doesn't work:


Aug 10 00:24:39 =================================== FAILURES ===================================
Aug 10 00:24:39 _____ TestUtilityFuns.test_validate_dynamic_axes_invalid_input_output_name _____
Aug 10 00:24:39 
Aug 10 00:24:39 self = <test_utility_funs.TestUtilityFuns testMethod=test_validate_dynamic_axes_invalid_input_output_name>
Aug 10 00:24:39 
Aug 10 00:24:39     def test_validate_dynamic_axes_invalid_input_output_name(self):
Aug 10 00:24:39         import warnings
Aug 10 00:24:39         with warnings.catch_warnings(record=True) as w:
Aug 10 00:24:39             utils._validate_dynamic_axes({'input1': {}, 'output': {},
Aug 10 00:24:39                                          'invalid_name1': {}, 'invalid_name2': {}},
Aug 10 00:24:39                                           None, ['input1', 'input2'], ['output'])
Aug 10 00:24:39             messages = [str(warning.message) for warning in w]
Aug 10 00:24:39 >       assert "Provided key invalid_name1 for dynamic axes is not a valid input/output name" in messages
Aug 10 00:24:39 E       AssertionError: assert 'Provided key invalid_name1 for dynamic axes is not a valid input/output name' in []

@MaximKalininMS
Copy link
Contributor Author

@ezyang Yes, I understand that :) However the same test passes in other configurations, e.g. caffe2_onnx_py3_6_clang7_ubuntu16_04_test. Actually these seem to be the only configurations where the test is running, so maybe python2-specific problem? I'll try to figure out.

@ezyang
Copy link
Contributor

ezyang commented Aug 12, 2019

We only run the onnx tests in the _onnx_ configuration ;)

@MaximKalininMS
Copy link
Contributor Author

@ezyang I fixed the test, the remaining CI issues seem unrelated to my change.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ezyang merged this pull request in 517b3c4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants