#574 Avoid Recomputing Schedules

[victorgarcia98]

Closes #574

[coveralls]

Pull Request Test Coverage Report for Build 4700657181

• 51 of 52 (98.08%) changed or added relevant lines in 5 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.2%) to 64.669%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
flexmeasures/data/services/utils.py	41	42	97.62%

Totals
Change from base Build 4688597922:	0.2%
Covered Lines:	6989
Relevant Lines:	10061

---

💛 - Coveralls

[Flix6x]

Very nice work! Just making sure: the job id itself is unaffected right? (We hand out the job id through the API as a return value for triggering a schedule.)

[victorgarcia98]

Very nice work! Just making sure: the job id itself is unaffected right? (We hand out the job id through the API as a return value for triggering a schedule.)

Yes! In the first trigger, we just return the Job. In the following triggers, we fetch the job and return it (the same job with the same job id).

[Flix6x]

This PR is coming along nicely. Don't forget to add the changelog entry. Other than that, it's just a matter of settling what to do with failed jobs.

[nhoening]

Cool stuff. I have some deeper questions and small asks.

[nhoening]

Just more typos :D

[Flix6x]

I made some commits myself, primarily to address my remaining issues with the documentation and code legibility. I tried to make them small, so you can follow my rationale behind each one.

Sorry about the unverified status of my commits (it seems like I somehow managed to make that worse than before).

While doing these I also came to the conclusion that the hash keys are not actually stored under some queue-specific segment of our Redis database. Therefore, I believe we can't be sure yet that the hash is unique if we apply the @job_cache decorator to more job-creating functions in the future. We have more functions that create jobs (at the moment, just 1 per queue I think), so that is something to be mindful of, but doesn't need to be resolved now (a possible resolution: perhaps the function that the job is supposed to execute can be added to the hash, and/or the queue name).

Another question that came up is whether there is any mechanism keeping the cache size at bay. Jobs have a limited lifetime, but here we are adding keys to Redis ourselves, and I suspect that the list just grows unlimited. Is that the case?

Finally, I found a if/else case for which no logic is specifically implemented. What happens (/ should happen) if the hash exists but the job id no longer exists in Redis (e.g. after the job's time to live has passed)?

[nhoening]

• I believe Redis keys have a TTL by design
• we could store the hash on the job meta data and create a post-hook function on jobs to delete that key when they are done or cancelled. Not on a laptop now, but I feel that RQ allows for that.

[Flix6x]

I believe Redis keys have a TTL by design

Alas, it looks like they don't. (source)

[nhoening]

Thanks. Then maybe my second idea would help.

[victorgarcia98]

Thanks for the reviews!

I've added the FLEXMEASURES_JOB_CACHE_TTL config variable to set a TTL for the caching keys.

Regarding the addition of the post-hook functions on_success and on_failiure, I think it's not needed because with the decorator we have the following functionality covered:

• In case of job failure: currently, with requeu=True, we requeu the same job after being called with the same arguments than before.
• In case of job success: we want to avoid recomputing the job to some extend. Moreover, on expiration of the the 'caching pair', the following call will trigger the creation of a new job.

In both cases, this behavior can be overridden by setting the flag force_new_job_creation to True, which forces the creation of a new Job.

With the post-hook functions, we had the following disadvantages:

• We had to define and add them them to each job creation function.
• We want to requeu the same job in case of a job failiure, If we delete the cache flag, we'll create a fresh new one (with a new Job Id).
• On success, we want to keep the cache flag so that repeating calls are cached.

Regarding the case that the Job has expired, we handle this as follow:

1. If the caching pair exists, we retrieve the Job Id
2. We check if it exists
   • If it does: we retrieve the Job object and return it.
   • If it doesn't: we exit both if's and proceed to create a new Job

Please, let me know If I'm missing something.

Regarding the hash collisions for functions with the same arguments, indeed that could happen. Even though unlikely, to avoid collisions in the future, I'm changing the key to be {queue_name}:{func_name}:{input_arguments_hash}.

[Flix6x]

I find that the new default TTL of an hour carries with it considerable weight, given that it scopes the whole caching logic. Two places I could think of that could mention that scope are: the changelog entries, and the documentation page for config variables. The new config variable should be added to that documentation page in any case.

[Flix6x]

🎉

[nhoening]

A very cool PR, thanks!