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

Ensure interactive pipeline is not re-executed needlessly #1029

Merged
merged 6 commits into from
Mar 12, 2023

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Mar 6, 2023

  • Fixes Interactive.eval() ensuring it returns the current value (previously it would only ever return the initial value)
  • Ensures that unless parameters change we don't re-evaluate the pipeline each time the value is requested by a callback.

Copy link
Member

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

Just left a comment in the code. Besides that I've tested the PR locally on a few examples and things seemed to work fine. So it's all good to me, merge it when you make the suggested change, if you think it makes sense.

Fixes Interactive.eval() ensuring it returns the current value (previously it would only ever return the initial value)

Indeed eval was broken, don't know we missed that until now!

Ensures that unless parameters change we don't re-evaluate the pipeline each time the value is requested by a callback.

That's the part I struggle to understand. Could you please explain what was going on before, with an example maybe?

One last comment, _setup_invalidation sets up the _invalidate_current callback on every Parameter of the pipeline, and it does so everytime Interactive.__init__ runs. So _invalidate_current in registered many times, that number grows quickly with the complexity and length of the pipeline. It is a pretty cheap function so I don't think it has any performance penalty. I just thought it was worth noting it as it might come as a bit unexpected and, the way interactive pipelines are built, iteratively for sure, it's likely users are going to register a lot of "orphan" callbacks, hopefully without any bad effects.

Co-authored-by: Maxime Liquet <35924738+maximlt@users.noreply.github.com>
@philippjfr
Copy link
Member Author

That's the part I struggle to understand. Could you please explain what was going on before, with an example maybe?

To be fully clear I'll preface this with the fact that the caching is still all-or-nothing, i.e. we do not in fact cache intermediate steps in a chain and then only evaluate the parts of the pipeline that have to be re-evaluated. Overall one might argue that the caching of intermediates may be too expensive anyway particularly for very long pipelines on large datasets. This is somewhat different from HoloViews where chaining generally results in a small number of steps and most operations are intended to reduce the size of the dataset.

Basically what happened before is that interactive held the original object and the series of transforms that have to be applied to arrive at the final object. Whenever the function returned by _callback was invoked it would then re-evaluate the whole pipeline (while eval just used the _current value). Since we never updated _current the eval method didn't work and because _callback would simply always re-evaluate the pipeline everything that required the output of the function would have to re-run the entire pipeline to get the current value.

What we do now is to internally invalidate the _current state so that when it is next required by .eval() or by a _callback we re-compute it and use that cached value until it is again invalidated.

@philippjfr
Copy link
Member Author

I've now cleaned up the invalidation code and updated the docstrings to explain the approach.

@philippjfr
Copy link
Member Author

Python 3.6 tests seem to be failing due to the solver consistently. Will ignore and merge.

@philippjfr philippjfr merged commit 8e561d1 into main Mar 12, 2023
@philippjfr philippjfr deleted the cache_interactive branch March 12, 2023 13:10
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