Skip to content

Conversation

@wchargin
Copy link
Contributor

Summary:
We’ve heretofore used the mock library without telling Bazel about it.
This isn’t generally a problem when tensorflow is installed in the
same virtualenv, because tensorflow pulls in mock. But tests that
don’t depend on TensorFlow have no build edge to mock when imported
into google3’s build system, causing build failures after syncing. This
commit makes the mock dependency explict.

We use mock==1.0.0 rather than the current version (2.0.0) because the
current version depends on pbr at runtime in a way that does not play
well with Bazel: testing-cabal/mock#383
(It appears to be trying to find some globally registered setup.cfg
file from which to read a version identifier.) Rather than vendoring and
patching mock, declaring a fake :expect_mock_installed target, or
hacking PBR_VERSION=2.0.0 into the action env, we simply depend on a
saner version of the library, which is also the version used in google3.

Existing mock users have been detected and fixed up:

$ bazel query "
>     kind(
>         py_.*rule,
>         rdeps(//..., set($(git grep -l 'import mock\($\| \)')), 1)
>     )
> " \
> | awk '{ print "add deps @org_pythonhosted_mock|" $0 }' \
> | buildozer -f - \
> ;

Test Plan:
All notf tests now pass in a virtualenv that does not include mock:

$ virtualenv -q -p python2 ./ve
$ . ./ve/bin/activate
(ve) $ pip install 'absl-py>=0.7.0' 'numpy<2.0,>=1.14.5' boto3==1.9.86 flake8==3.5.0 futures==3.1.1 grpcio==1.6.3 yamllint==1.5.0 >/dev/null 2>&1
(ve) $ python -c 'import mock'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/local/buildtools/current/sitecustomize/sitecustomize.py", line 152, in SetupPathsAndImport
    return real_import(name, globals, locals, fromlist, level)
ImportError: No module named mock
(ve) $ cd ~/git/tensorboard
(ve) $ bazel test //tensorboard/... --test_tag_filters=support_notf
...
INFO: Build completed successfully, 12 total actions
//tensorboard:lazy_test                                                  PASSED in 0.2s
//tensorboard:lib_test                                                   PASSED in 0.9s
//tensorboard:manager_test                                               PASSED in 0.3s
//tensorboard:plugin_util_test                                           PASSED in 0.5s
//tensorboard:program_test                                               PASSED in 0.8s
//tensorboard/backend:application_test                                   PASSED in 1.0s
//tensorboard/backend:http_util_test                                     PASSED in 0.8s
//tensorboard/backend:json_util_test                                     PASSED in 0.8s
//tensorboard/compat/tensorflow_stub:gfile_test                          PASSED in 0.8s
//tensorboard/summary:summary_test                                       PASSED in 0.8s
//tensorboard/util:platform_util_test                                    PASSED in 0.3s

Previously, manager_test and application_test failed in such an
environment.

wchargin-branch: mock-dep

Summary:
We’ve heretofore used the `mock` library without telling Bazel about it.
This isn’t generally a problem when `tensorflow` is installed in the
same virtualenv, because `tensorflow` pulls in `mock`. But tests that
don’t depend on TensorFlow have no build edge to `mock` when imported
into google3’s build system, causing build failures after syncing. This
commit makes the `mock` dependency explict.

We use `mock==1.0.0` rather than the current version (2.0.0) because the
current version depends on `pbr` at runtime in a way that does not play
well with Bazel: <testing-cabal/mock#383>
(It appears to be trying to find some globally registered `setup.cfg`
file from which to read a version identifier.) Rather than vendoring and
patching `mock`, declaring a fake `:expect_mock_installed` target, or
hacking `PBR_VERSION=2.0.0` into the action env, we simply depend on a
saner version of the library, which is also the version used in google3.

Existing `mock` users have been detected and fixed up:

```
$ bazel query "
>     kind(
>         py_.*rule,
>         rdeps(//..., set($(git grep -l 'import mock\($\| \)')), 1)
>     )
> " \
> | awk '{ print "add deps @org_pythonhosted_mock|" $0 }' \
> | buildozer -f - \
> ;
```

Test Plan:
All notf tests now pass in a virtualenv that does not include `mock`:

```
$ virtualenv -q -p python2 ./ve
$ . ./ve/bin/activate
(ve) $ pip install 'absl-py>=0.7.0' 'numpy<2.0,>=1.14.5' boto3==1.9.86 flake8==3.5.0 futures==3.1.1 grpcio==1.6.3 yamllint==1.5.0 >/dev/null 2>&1
(ve) $ python -c 'import mock'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/local/buildtools/current/sitecustomize/sitecustomize.py", line 152, in SetupPathsAndImport
    return real_import(name, globals, locals, fromlist, level)
ImportError: No module named mock
(ve) $ cd ~/git/tensorboard
(ve) $ bazel test //tensorboard/... --test_tag_filters=support_notf
...
INFO: Build completed successfully, 12 total actions
//tensorboard:lazy_test                                                  PASSED in 0.2s
//tensorboard:lib_test                                                   PASSED in 0.9s
//tensorboard:manager_test                                               PASSED in 0.3s
//tensorboard:plugin_util_test                                           PASSED in 0.5s
//tensorboard:program_test                                               PASSED in 0.8s
//tensorboard/backend:application_test                                   PASSED in 1.0s
//tensorboard/backend:http_util_test                                     PASSED in 0.8s
//tensorboard/backend:json_util_test                                     PASSED in 0.8s
//tensorboard/compat/tensorflow_stub:gfile_test                          PASSED in 0.8s
//tensorboard/summary:summary_test                                       PASSED in 0.8s
//tensorboard/util:platform_util_test                                    PASSED in 0.3s
```

Previously, `manager_test` and `application_test` failed in such an
environment.

wchargin-branch: mock-dep
@wchargin
Copy link
Contributor Author

Googlers: http://cl/244441864 is the corresponding Copybara change.

Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

Thank you!! 🥝

@wchargin wchargin marked this pull request as ready for review April 20, 2019 00:35
wchargin-branch: mock-dep
@wchargin wchargin merged commit 90a386d into master Apr 20, 2019
@wchargin
Copy link
Contributor Author

cc @orionr, in case you add more support_notf tests and run into this.
No action required.

@wchargin wchargin deleted the wchargin-mock-dep branch April 20, 2019 00:59
wchargin added a commit that referenced this pull request Oct 25, 2019
Summary:
Some of the `//tensorboard/uploader/...` tests are currently disabled in
Python 2 because our backport of `mock` doesn’t support the required
assertions. We can’t easily upgrade the dependency (see #2132), but we
can backport those assertions. The assertions backported are:

  - `assert_not_called`
  - `assert_called`
  - `assert_called_once`

This patch is the combination of (restrictions of) the following
upstream Git commits:

  - [`c7d6c7dcafd4482626194528b24f174dd15f5f5e`][1] (2014-04-17)
  - [`8882c2fccb9f7d3b8ba2b5534c0dd9d52343725f`][2] (2014-04-17)
  - [`7ca5d3afe293d5e4d769c1cb2dfccb0688ba1292`][3] (2016-03-11)
  - [`07532a75003ec8e871e494f9f1ab8f28fa246f08`][4] (2018-01-19)

I identified those commits by diffing our pinned version of `mock.py`
(the version at upstream commits `b14e284` through `dc1458a`) against
the version of `mock/mock.py` in the current repo, tracing the blame on
the implementation of the relevant assertions, then pruning commits that
relied on other changes not yet patched in (and were [only cosmetic,
anyway][5]).

[1]: testing-cabal/mock@c7d6c7d
[2]: testing-cabal/mock@8882c2f
[3]: testing-cabal/mock@7ca5d3a
[4]: testing-cabal/mock@07532a7
[5]: testing-cabal/mock@57ef3bb

Test Plan:
Unit tests pass in Python 2, and are now actually run, too. Changing
`uploader_test.py`’s `test_upload_swallows_rpc_failure` to comment out
the `_upload_once` call causes the subsequent `assert_called_once` to
fail with the correct error message, showing that the assertions are
wired up properly.

wchargin-branch: mock-backport
wchargin added a commit that referenced this pull request Oct 25, 2019
Summary:
Some of the `//tensorboard/uploader/...` tests are currently disabled in
Python 2 because our backport of `mock` doesn’t support the required
assertions. We can’t easily upgrade the dependency (see #2132), but we
can backport those assertions. The assertions backported are:

  - `assert_not_called`
  - `assert_called`
  - `assert_called_once`

This patch is the combination of (restrictions of) the following
upstream Git commits:

  - [`c7d6c7dcafd4482626194528b24f174dd15f5f5e`][1] (2014-04-17)
  - [`8882c2fccb9f7d3b8ba2b5534c0dd9d52343725f`][2] (2014-04-17)
  - [`7ca5d3afe293d5e4d769c1cb2dfccb0688ba1292`][3] (2016-03-11)
  - [`07532a75003ec8e871e494f9f1ab8f28fa246f08`][4] (2018-01-19)

I identified those commits by diffing our pinned version of `mock.py`
(the version at upstream commits `b14e284` through `dc1458a`) against
the version of `mock/mock.py` in the current repo, tracing the blame on
the implementation of the relevant assertions, then pruning commits that
relied on other changes not yet patched in (and were [only cosmetic,
anyway][5]).

[1]: testing-cabal/mock@c7d6c7d
[2]: testing-cabal/mock@8882c2f
[3]: testing-cabal/mock@7ca5d3a
[4]: testing-cabal/mock@07532a7
[5]: testing-cabal/mock@57ef3bb

Test Plan:
Unit tests pass in Python 2, and are now actually run, too. Changing
`uploader_test.py`’s `test_upload_swallows_rpc_failure` to comment out
the `_upload_once` call causes the subsequent `assert_called_once` to
fail with the correct error message, showing that the assertions are
wired up properly.

wchargin-branch: mock-backport
wchargin added a commit to wchargin/tensorboard that referenced this pull request Oct 29, 2019
Summary:
Some of the `//tensorboard/uploader/...` tests are currently disabled in
Python 2 because our backport of `mock` doesn’t support the required
assertions. We can’t easily upgrade the dependency (see tensorflow#2132), but we
can backport those assertions. The assertions backported are:

  - `assert_not_called`
  - `assert_called`
  - `assert_called_once`

This patch is the combination of (restrictions of) the following
upstream Git commits:

  - [`c7d6c7dcafd4482626194528b24f174dd15f5f5e`][1] (2014-04-17)
  - [`8882c2fccb9f7d3b8ba2b5534c0dd9d52343725f`][2] (2014-04-17)
  - [`7ca5d3afe293d5e4d769c1cb2dfccb0688ba1292`][3] (2016-03-11)
  - [`07532a75003ec8e871e494f9f1ab8f28fa246f08`][4] (2018-01-19)

I identified those commits by diffing our pinned version of `mock.py`
(the version at upstream commits `b14e284` through `dc1458a`) against
the version of `mock/mock.py` in the current repo, tracing the blame on
the implementation of the relevant assertions, then pruning commits that
relied on other changes not yet patched in (and were [only cosmetic,
anyway][5]).

[1]: testing-cabal/mock@c7d6c7d
[2]: testing-cabal/mock@8882c2f
[3]: testing-cabal/mock@7ca5d3a
[4]: testing-cabal/mock@07532a7
[5]: testing-cabal/mock@57ef3bb

Test Plan:
Unit tests pass in Python 2, and are now actually run, too. Changing
`uploader_test.py`’s `test_upload_swallows_rpc_failure` to comment out
the `_upload_once` call causes the subsequent `assert_called_once` to
fail with the correct error message, showing that the assertions are
wired up properly.

wchargin-branch: mock-backport
wchargin added a commit that referenced this pull request Oct 29, 2019
Summary:
Some of the `//tensorboard/uploader/...` tests are currently disabled in
Python 2 because our backport of `mock` doesn’t support the required
assertions. We can’t easily upgrade the dependency (see #2132), but we
can backport those assertions. The assertions backported are:

  - `assert_not_called`
  - `assert_called`
  - `assert_called_once`

This patch is the combination of (restrictions of) the following
upstream Git commits:

  - [`c7d6c7dcafd4482626194528b24f174dd15f5f5e`][1] (2014-04-17)
  - [`8882c2fccb9f7d3b8ba2b5534c0dd9d52343725f`][2] (2014-04-17)
  - [`7ca5d3afe293d5e4d769c1cb2dfccb0688ba1292`][3] (2016-03-11)
  - [`07532a75003ec8e871e494f9f1ab8f28fa246f08`][4] (2018-01-19)

I identified those commits by diffing our pinned version of `mock.py`
(the version at upstream commits `b14e284` through `dc1458a`) against
the version of `mock/mock.py` in the current repo, tracing the blame on
the implementation of the relevant assertions, then pruning commits that
relied on other changes not yet patched in (and were [only cosmetic,
anyway][5]).

[1]: testing-cabal/mock@c7d6c7d
[2]: testing-cabal/mock@8882c2f
[3]: testing-cabal/mock@7ca5d3a
[4]: testing-cabal/mock@07532a7
[5]: testing-cabal/mock@57ef3bb

Test Plan:
Unit tests pass in Python 2, and are now actually run, too. Changing
`uploader_test.py`’s `test_upload_swallows_rpc_failure` to comment out
the `_upload_once` call causes the subsequent `assert_called_once` to
fail with the correct error message, showing that the assertions are
wired up properly.

wchargin-branch: mock-backport
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants