Only cache things in a registry #1068
Conversation
|
@tcstewar mentioned he is using the cache to cache something else. Not sure if that use is affected by this? The downside I see with this approach is that there
Except that there is still the I think both approaches have their advantages and disadvantages and overall I don't have a strong preference either way. |
Yes, I considered having these in
I agree, pros and cons to both! I have an aversion to modifying user-facing objects whenever possible, and would like to reduce the surface area of the API (i.e., not introduce new classes like |
I don't know ... btw: Not sure if I mentioned this, but my approach was inspired by how the for example pickle allows objects to define the own pickling function. |
nengo_spinnaker uses the cache! |
Yup! I think this is another situation like the Parameter system where we need to be careful about changes because other backends are using this too... nengo_brainstorm also can use the cache, and in a weird way.... |
|
I pushed a commit that moves all the fingerprinting related code to |
|
I looked at this again and noticed that 9074d8c removed the dependence on solver internals. I added another commit to take this a step further and not just check attributes of the solver that are instances of |
|
I think I'm ok with and without that additional commit. I think the additional commit makes this more conservative because it checks more. |
|
Interesting! I'm 👍 on 64cf8f3. Pushed a fixup to remove some unused imports and space things out a bit more (is more readable for me, at least, but maybe I've been doing too much JavaScript). There are some flake8 errors in this branch, but they will be resolved once it's rebased to master. |
Definitely nice when adding/removing things because you don't need to reformat everything to fit the line length. Only downside I see is somewhat more scrolling. |
| (np.ndarray, check_dtype), | ||
| (tuple, check_seq), | ||
| (list, check_seq) | ||
| ] + [ |
There was a problem hiding this comment.
There's no :barf: emoji but if there was I would use it on this line just gonna settle for 😱
|
I'm shrugs on the reformatting; not how I would write it myself, but I think it's fine either way. And the rest all looks 👍 to me. |
Addresses #1054. Code can change and we can't detect it, so we allow only things that are marked as supporting fingerprints to be cached and only mark objects that should be stable as supporting this. This also removes the solver_fn from the fingerprint because it is not needed in there. It should always be the same function which is part of the builder (the solver itself is still part of the fingerprint of course). This in turn allows to simplify the SolverMock in the test code a little bit. (It actually is slightly misnamed as it mocks the solver_fn and not a Solver.) Co-authored by: Jan Gosmann <jgosmann@uwaterloo.ca> Co-authored by: Daniel Rasmussen <daniel.rasmussen@appliedbrainresearch.com>
drasmuss
force-pushed
the
fix-1054-registry
branch
from
3f058a6
to
33829ee
Compare
This is an alternative implementation of #1066 that uses a class registry in the
Fingerprintrather than modifying frontend objects.It occurred to me that the cache system (at least for now) is specific to the reference backend, so it would be best to keep any cache-specific code local to the cache, rather than modifying frontend objects. So that's what this branch does.
I think the amount of code is, in the end, about the same.
Fingerprint.__init__is more clear this way (IMO) but we don't get the benefit of subclassing inheriting things, so each class that should be fingerprinted needs a separate call toFingerprint.whitelist. But perhaps that's desirable anyhow?