Skip to content
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

make torch.testing asserts importable #54769

Closed
wants to merge 6 commits into from
Closed

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Mar 26, 2021

Stack from ghstack:

Follow-up to #53820. This

  • makes the asserts.py module private as per suggestion from @rgommers in initial draft for assert_tensors_(equal|allclose) in torch.testing #53820 (comment). With this the functions should only be accessible through torch.testing, giving us the option the change the underlying structure later.
  • moves the code from torch/testing/__init__.py to torch/testing/_core.py (happy to accept other name suggestions). Otherwise we can't import the new _asserts.py in torch/testing/__init__.py due to circular imports.
  • factors out some functionality from torch/testing/_internal/common_utils.py that is needed in new _asserts.py to torch/testing/_internal/testing_utils.py to not be dependent on numpy at import.

Differential Revision: D27438451

move coda out of torch.testing.__init__
make everything importable again


delete torch/testing/asserts.py


please flake8


provide more stuff in torch.testing


provide even more stuff in torch.testing


expose more


fix import


make private


expose more


fix mypy

[ghstack-poisoned]
@facebook-github-bot facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Mar 26, 2021
pmeier added a commit that referenced this pull request Mar 26, 2021
move coda out of torch.testing.__init__
make everything importable again


delete torch/testing/asserts.py


please flake8


provide more stuff in torch.testing


provide even more stuff in torch.testing


expose more


fix import


make private


expose more


fix mypy

ghstack-source-id: 6c34c237c4d8c0d248edd66a329aefc7260b8723
Pull Request resolved: #54769
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 26, 2021

💊 CI failures summary and remediations

As of commit 436356e (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

move coda out of torch.testing.__init__
make everything importable again


delete torch/testing/asserts.py


please flake8


provide more stuff in torch.testing


provide even more stuff in torch.testing


expose more


fix import


make private


expose more


fix mypy

[ghstack-poisoned]
pmeier added a commit that referenced this pull request Mar 26, 2021
Follow-up to #53820. This

- makes the `asserts.py` module private as per suggestion from @rgommers in #53820 (comment). With this the functions should only be accessible through `torch.testing`, giving us the option the change the underlying structure later.
- moves the code from `torch/testing/__init__.py` to `torch/testing/_core.py` (happy to accept other name suggestions). Otherwise we can't import the new `_asserts.py` in `torch/testing/__init__.py` due to circular imports.

ghstack-source-id: 3b9dfacf4bf5f01b827276be4c3724bec6745e9f
Pull Request resolved: #54769
@pmeier pmeier requested a review from mruberry March 26, 2021 17:59
@pmeier
Copy link
Collaborator Author

pmeier commented Mar 26, 2021

@mruberry Not sure about the test failures. For some reason importing numpy is failing. From the error message

ModuleNotFoundError: Sorry PyTorch, but our NumPy is in the other folder

it seems someone planned for this. Do you have insights?

@mruberry
Copy link
Collaborator

@mruberry Not sure about the test failures. For some reason importing numpy is failing. From the error message

ModuleNotFoundError: Sorry PyTorch, but our NumPy is in the other folder

it seems someone planned for this. Do you have insights?

I'd spend some more time debugging (possibly after a rebase for good measure). It looks like two tests are failing. The ModuleNotFoundError is coming from a "fake NumPy" that was added for testing purposes in February. What's causing it to fail?

@pmeier
Copy link
Collaborator Author

pmeier commented Mar 29, 2021

#52794 added a test to check that torch is importable

if PyTorch was compiled with wrong NumPy version, of NumPy is not commonly available on the platform (which is often the case on AARCH64 or Apple M1)

The new _asserts.py needs functionality from torch.testing._internal which imports numpy unconditionally. Since this has not happened before, the tests went fine. I'll factor out the functionality that we need into a separate file.

Follow-up to #53820. This

- makes the `asserts.py` module private as per suggestion from @rgommers in #53820 (comment). With this the functions should only be accessible through `torch.testing`, giving us the option the change the underlying structure later.
- moves the code from `torch/testing/__init__.py` to `torch/testing/_core.py` (happy to accept other name suggestions). Otherwise we can't import the new `_asserts.py` in `torch/testing/__init__.py` due to circular imports.

[ghstack-poisoned]
Follow-up to #53820. This

- makes the `asserts.py` module private as per suggestion from @rgommers in #53820 (comment). With this the functions should only be accessible through `torch.testing`, giving us the option the change the underlying structure later.
- moves the code from `torch/testing/__init__.py` to `torch/testing/_core.py` (happy to accept other name suggestions). Otherwise we can't import the new `_asserts.py` in `torch/testing/__init__.py` due to circular imports.

[ghstack-poisoned]
@pmeier pmeier requested review from mruberry and removed request for mruberry March 29, 2021 09:21
Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Cool; so this also fixes that importing torch imports numpy?

I wonder if we should add a test for that?

@mruberry
Copy link
Collaborator

#52794 added a test to check that torch is importable

if PyTorch was compiled with wrong NumPy version, of NumPy is not commonly available on the platform (which is often the case on AARCH64 or Apple M1)

The new _asserts.py needs functionality from torch.testing._internal which imports numpy unconditionally. Since this has not happened before, the tests went fine. I'll factor out the functionality that we need into a separate file.

I like the comment you made in the code about this. Would you update this PR description, too? We should talk about our plan for the structure of all this code at our next testing sync next week.

@pmeier
Copy link
Collaborator Author

pmeier commented Mar 31, 2021

Cool; so this also fixes that importing torch imports numpy?

Correct.

I wonder if we should add a test for that?

The test that failed before tests for exactly that:

test_without_numpy() {
pushd "$(dirname "${BASH_SOURCE[0]}")"
python -c "import sys;sys.path.insert(0, 'fake_numpy');from unittest import TestCase;import torch;x=torch.randn(3,3);TestCase().assertRaises(RuntimeError, lambda: x.numpy())"
popd
}

Still, the error message through me off

raise ModuleNotFoundError("Sorry PyTorch, but our NumPy is in the other folder")

Maybe we can change it into something more descriptive like

raise ModuleNotFoundError("Importing 'torch' must not require 'numpy' to be present.")

Cc @malfet

@mruberry
Copy link
Collaborator

I need to investigate this further internally. The new structure either requires some internal tweaks or there's a problem with the specifying the imports relative to the current location.

Example failure:

  File ".../tree/torch/testing/_asserts.py", line 7, in <module>
    from ._internal.testing_utils import (
ModuleNotFoundError: No module named 'torch.testing._internal'

Follow-up to #53820. This

- makes the `asserts.py` module private as per suggestion from @rgommers in #53820 (comment). With this the functions should only be accessible through `torch.testing`, giving us the option the change the underlying structure later.
- moves the code from `torch/testing/__init__.py` to `torch/testing/_core.py` (happy to accept other name suggestions). Otherwise we can't import the new `_asserts.py` in `torch/testing/__init__.py` due to circular imports.
- factors out some functionality from `torch/testing/_internal/common_utils.py` that is needed in new `_asserts.py` to `torch/testing/_internal/testing_utils.py` to not be dependent on `numpy` at import.

Differential Revision: [D27438451](https://our.internmc.facebook.com/intern/diff/D27438451)

[ghstack-poisoned]
@mruberry
Copy link
Collaborator

mruberry commented Apr 5, 2021

I had some more time to investigate the internal build system, and I think the issue is simply that the current build system isn't designed to allow _internal to be imported into testing, since torch.testing is always imported but torch.testing._internal is only imported on test builds.

We could try and tweak the internal build system, but I'd prefer we just work around the issue. Can we just copy parts of _internal we need for asserts.py into torch/testing/asserts.py for now, for example?

Follow-up to #53820. This

- makes the `asserts.py` module private as per suggestion from @rgommers in #53820 (comment). With this the functions should only be accessible through `torch.testing`, giving us the option the change the underlying structure later.
- moves the code from `torch/testing/__init__.py` to `torch/testing/_core.py` (happy to accept other name suggestions). Otherwise we can't import the new `_asserts.py` in `torch/testing/__init__.py` due to circular imports.
- factors out some functionality from `torch/testing/_internal/common_utils.py` that is needed in new `_asserts.py` to `torch/testing/_internal/testing_utils.py` to not be dependent on `numpy` at import.

Differential Revision: [D27438451](https://our.internmc.facebook.com/intern/diff/D27438451)

[ghstack-poisoned]
@pmeier
Copy link
Collaborator Author

pmeier commented Apr 6, 2021

Can we just copy parts of _internal we need for asserts.py into torch/testing/asserts.py for now, for example?

Yes, we can. I've removed the torch/testing/_internal/testing_utils.py again and simply copied the dtype precisions. Note that copying get_comparison_dtype was not necessary, since the todos are outdated.

def get_comparison_dtype(a, b):
# TODO: update this when promote_types supports bfloat16 and/or
# isclose supports bfloat16.
a_dtype = torch.float32 if a.dtype is torch.bfloat16 else a.dtype
b_dtype = torch.float32 if b.dtype is torch.bfloat16 else b.dtype
compare_dtype = torch.promote_types(a_dtype, b_dtype)
# non-CUDA (CPU, for example) float16 -> float32
# TODO: update this when isclose is implemented for CPU float16
if (compare_dtype is torch.float16 and
(a.device != b.device or a.device.type != 'cuda' or
b.device.type != 'cuda')):
compare_dtype = torch.float32
return compare_dtype

Thus, it can simply replaced by torch.promote_types().

@mruberry
Copy link
Collaborator

mruberry commented Apr 7, 2021

Let's see what the internal build system thinks.

@facebook-github-bot
Copy link
Contributor

@mruberry merged this pull request in f4967d6.

pmeier added a commit that referenced this pull request Apr 12, 2021
* #54769 make torch.testing asserts importable

[ghstack-poisoned]
pmeier added a commit that referenced this pull request Apr 12, 2021
* #54769 make torch.testing asserts importable

[ghstack-poisoned]
facebook-github-bot pushed a commit that referenced this pull request Apr 19, 2021
Summary:
Pull Request resolved: #54784

* #54769 make torch.testing asserts importable

Test Plan: Imported from OSS

Reviewed By: jbschlosser

Differential Revision: D27717422

Pulled By: mruberry

fbshipit-source-id: 7526af4f17d8ffcc4ea5e5a5d98f07ceac89df40
krshrimali pushed a commit to krshrimali/pytorch that referenced this pull request May 19, 2021
Summary:
Pull Request resolved: pytorch#54784

* pytorch#54769 make torch.testing asserts importable

Test Plan: Imported from OSS

Reviewed By: jbschlosser

Differential Revision: D27717422

Pulled By: mruberry

fbshipit-source-id: 7526af4f17d8ffcc4ea5e5a5d98f07ceac89df40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged oncall: jit Add this issue/PR to JIT oncall triage queue open source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants