Skip to content

Conversation

@wchargin
Copy link
Contributor

Follow-up to #1781. See individual commit messages for details. To be
merged with rebase.

@wchargin wchargin requested a review from nfelt January 30, 2019 18:36
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional but seems like you might as well put this logic in _memoize itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But _memoize takes a function that accepts a single hashable argument,
so recursion is perfectly fine. If _memoize took a niladic function,
the argument would be more convincing, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be per-argument, e.g.

def _memoize(f):
  """Memoizing decorator for f, which must have exactly 1 hashable argument."""
  nothing = object()  # Unique "no value" sentinel object.
  loading = object()  # Unique "loading" sentinel object.
  cache = {}
  # Use a reentrant lock to allow f() to reference itself.
  lock = threading.RLock()
  @functools.wraps(f)
  def wrapper(arg):
    if cache.get(arg, nothing) == nothing:
      with lock:
        current = cache.get(arg, nothing)
        if current == loading:
          raise RuntimeException("circular reference")
        elif current == nothing:
          cache[arg] = loading
          cache[arg] = f(arg)
    return cache[arg]
  return wrapper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don’t mind, I’ll keep it as written in this PR, because that
enables us to give a better error message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Update comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes; thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

six.assertRaisesRegex()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure; will change.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO self.fail("...") slightly more idiomatic for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed; will change.

Summary:
This commit adds a test case for [behavior implemented in #1781][1].

[1]: #1781 (comment)

Test Plan:
`bazel test //tensorboard:lazy_test` passes at HEAD, but fails after
checking out `lazy.py` from 976064d.

wchargin-branch: lazy-composition
Summary:
This tests behavior [described in #1781][2], improving the error message
along the way.

[2]: #1781 (comment)

Test Plan:
`bazel test //tensorboard:lazy_test` passes at HEAD, but changing the
implementation of `_memoize` to use `Lock` instead of `RLock` causes a
deadlock (and so the test times out and fails eventually).

wchargin-branch: lazy-cycles
@wchargin wchargin force-pushed the wchargin-lazy-tweaks branch from 438935d to 2cbdc48 Compare January 30, 2019 22:50
Summary:
It was [noted in #1781][3] that the `__repr__` of `LazyModule`s deserves
a modicum of special attention. This commit tests that, and improves the
`__repr__` for already-loaded lazy modules so that it’s clear that they
are still going through the `LazyModule` proxy. (Should there be a bug
in the proxy interface—say, someone expects `__setattr__` to also pass
through to the underlying module—this would make it much easier to track
down.)

[3]: #1781 (review)

Test Plan:
`bazel test //tensorboard:lazy_test` passes.

wchargin-branch: lazy-repr
@wchargin wchargin force-pushed the wchargin-lazy-tweaks branch from 2cbdc48 to 02493b9 Compare January 31, 2019 22:09
@wchargin wchargin merged commit 04e13f6 into master Jan 31, 2019
@wchargin wchargin deleted the wchargin-lazy-tweaks branch January 31, 2019 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants