Lunderberg commented on pull request #7:
URL: https://github.com/apache/tvm-rfcs/pull/7#issuecomment-870833178
Copying some notes from a 1:1, along with commentary at the bottom.
Caching options listed below, arranged from most flexibility at the top to
least flexibility at the bottom.
1. All caching allowed. A fixture tagged with `cache_return_value=True` can
return any python object. All tests that use the fixture receive the same
python object as input.
- Pros
- Most potential to speed up running of CI.
- Most potential to speed up by cross-function/file use of shared
setup (e.g. np.random.uniform)
- Works for any type.
- Cons
- Most potential to have interacting unit tests.
- Could make somebody think that interactions are allowed between
unit tests.
2. Cache only copy-able types. A fixture tagged with
`cache_return_value=True` has cached return values, but each parametrized unit
test receives a copy made by `copy.deepcopy` prior to returning the object.
Any types that raise an error during copying result in a failed setup for the
test.
- Pros
- Very simple to add caching to existing functions.
- Cons
- Unintentionally copy global state that is pointed to by local
state. Doesn't impact correctness, but would increase runtime.
- Less flexibility, an object that cannot be deepcopy-ed results in
a type error.
3. Cache only copy-able types, but with a custom implementation of
`copy.deepcopy`. Would follow the same rules if a `__deepcopy__` or pair of
`__getstate__`/ `__setstate__` exists, but would otherwise require the type to
be in a list of allowed types. List of types
- Pros
- Opt-in both by fixture and by data-type.
- Cons
- Re-implements a common utility.
4. Cache using explicitly specified serialize/deserialize functions. A
fixture tagged with `cache_return_value=True` must return three values. The
first is the fixture value itself. The second/third are serialize/deserialize
functions for that value.
- Pros
- Cons
5. Only caching of `bytes` type is allowed. A fixture tagged with
`cache_return_value=True` must return a `bytes` object. This object is
deserialized at the start of the test function into the desired type.
- Pros
- Cons
6. No caching. All fixtures are recomputed for each parametrized value.
- Pros
- All unit tests are independent by design. Zero interaction
possible.
- Cons
- Most potential impact to CI runtime.
Overall, @areusch 's concern is avoiding hidden failure modes, especially
where there could be tests that only appear to pass due to cross-talk. As
such, he is more comfortable with more explicit usage. and would be comfortable
with any of implementations 3-5. @Lunderberg 's main concern is usability and
ease of enabling, such that a slow test can have caching enabled with minimal
effort where it is needed, and would be comfortable with any of implementations
2-3. One key point that we came up with is that we expect fixtures to be
uncached by default (`@tvm.testing.fixture`), and to have caching enabled on a
per-fixture basis (`@tvm.testing.fixture(cache_return_value=True)` where there
are performance benefits to be gained.
Since #3 is the overlap we both like between having opt-in caching, both on
a per-return-value and per-fixture basis, I've put together [a test
implementation](https://gist.github.com/Lunderberg/039f2f1d14c9ad7de88e1d2224a430cc).
It calls `copy.deepcopy`, but passes in a custom `memo` argument that
performs type-checking of the copied type. Types can be explicitly listed as
being safe to copy, but the use of the default `object.__reduce__` is disabled.
This implementation minimizes the amount of re-implementing that would need to
be done, but does rely on `ctypes` to identify the object being checked.
@areusch Can you check this over to make sure I've given an accurate
description of your comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
