Make reload.project() compatible with clear() and clear.cache()

connectedblue commented 7 years ago

This PR proposes an extension to the reload.project() function to take account of recent additions to ProjectTemplate in a more natural way.

The existing behaviour is to clear the global environment and then run load.project() again. The PR essentially does the same but takes account of more factors:

There is a new parameter config$sticky_variables which are for specific datasets that may take a long time to load and the user doesn't want them cleared during a clear(). These are now also honoured during reload.project()
There is a new paramer reset so if reload.project(reset=TRUE) is called, then everything is cleared from both Global env and cache regardless (the project is reset).
- The parameter ... is used to capture any additonal parameters to pass to load.project (e.g. override.config). This was hard coded before, but it's more generic to use the dots notation.

The documentation for reload.project() has been updated. I couldn't see any relevant parts of the website documentation that should be updated.

KentonWhite commented 7 years ago

I agree that the expected behavior is that all data is removed and reloaded from scratch. I'm a bit worried about removing cache items using reload.project().

Let's imagine the user with all of their long loading data stored in cache. After an update they run reload.project() expecting the old behavior and for whatever reason some of those variables are missing from the global environment. reload.project() now wipes out their cache and they are stuck with a long rebuild time.

If we are removing items from cache with reload.project() I'd feel more comfortable if we provide a confirmation for cached items that will be removed. This way a user can prevent a disastrous mistake!

connectedblue commented 7 years ago

I see your point.

Can we break down the cases between how it might work going forward, and what a user expects after updating to the latest ProjectTemplate? We can help the latter through migrate.project if we detect they have things in the cache.

So ..... should the new behaviour be:

if I reload.project I want everything re-calculated from scratch, so clear both memory and cache
if I have declared certain variables to be protected in config$sticky_variables, then I want them to remain in memory and/or cache during a reload
Even if I have declared config$sticky_variables, I want to consciously erase everything anyway, so I set reload.project(reset=TRUE)

I think points 2 and 3 are not contentious - the user is in control of what's happening.

With point 1, it boils down to whether most of the time users expect to have both clear() and clear.cache() when doing a reload.

I did toy with the idea of creating a reset.project() function which did both and leave reload.project() to just clear global env - but this seems even more confusing for a user to have to remember.

On balance, I'm agnostic on clearing the cache in case 1. I can see the benefits, and I can see the other side. Let me rework the PR to just use clear(). We can always add clear.cache() in later if required.

Are you still comfortable with case 3 - having a reset parameter explicitly set to clear everything?

connectedblue commented 7 years ago

OK, this is the reworked PR for review. I also edited the original post above for the record so it describes what's now implemented.

The behaviour is essentially the same as before, except that config$sticky_variables is honoured and there is a new reset parameter to allow a complete scrub out and reload if that's what the user wants.

KentonWhite commented 7 years ago

I like the notation reload.project(reset = TRUE). It allows us to keep reload.project working as it has (reload the project, including reloading the cached data) and start moving towards a future where reload.project recalculates all cached variables.

I would also add a test to see if the user explicitly passes reset. If they don't pass a reset variable, provide a warning describing the new feature and that in a later release the default will change from FALSE to TRUE.

Also curious why you closed this PR?

connectedblue commented 7 years ago

Hi @KentonWhite. I like this idea - a warning of future deprecation. I'll have a go at that. I'm not really sure how you test if a user actually specified a variable (other than checking for the default value), but I'll have a google around.

I closed the PR because I didn't want it merged until I've reviewed the tests. I haven't got a lot of time now that the New Year is underway, so I'm not sure I'll get it done over the next few days. I also want to add in your suggestion, so that'll be a bit longer again.

connectedblue commented 7 years ago

Added the check suggested by @KentonWhite if the cache is not empty.

PR ready for review now.

connectedblue commented 7 years ago

Hi @KentonWhite - have you had a chance to review this PR? It was updated to reflect your last comments back in January.

I've been rather busy at work since then, so haven't had a lot of time to look at ProjectTemplate recently.

KentonWhite commented 7 years ago

Sorry hadn't seen that it had been re-opened

KentonWhite / ProjectTemplate

Make reload.project() compatible with clear() and clear.cache() #183