Move sensor data ingestion to job queue#2101
Conversation
Documentation build overview
25 files changed ·
|
Agent-Logs-Url: https://github.com/FlexMeasures/flexmeasures/sessions/d8f1f856-844d-4274-a34b-36e33e6d8f69 Co-authored-by: joshuaunity <45713692+joshuaunity@users.noreply.github.com>
Agent-Logs-Url: https://github.com/FlexMeasures/flexmeasures/sessions/d8f1f856-844d-4274-a34b-36e33e6d8f69 Co-authored-by: joshuaunity <45713692+joshuaunity@users.noreply.github.com>
|
@copilot, add a proper step-by-step guide on how to test this |
Updated the PR description with a step-by-step testing guide covering both the queued path (with an ingestion worker) and the synchronous fallback path (no workers). See the "How to test" section below. |
|
This works from my tests From 2.5+ sec to about 300ms The fallback (run synchronously if no available worker) also works nicely without any issues |
joshuaunity
left a comment
There was a problem hiding this comment.
This works pretty well, and the code is clean and straightforward.
@nhoening, are there any additions you would like to make to this?
So basically you are asking for an early review :) I also like the code and that there is some documentation already, as well. And the fallback is good.
|
Agent-Logs-Url: https://github.com/FlexMeasures/flexmeasures/sessions/a05e0a24-6919-4080-a466-cbaf6a8369de Co-authored-by: joshuaunity <45713692+joshuaunity@users.noreply.github.com>
Yes, they are all affected and will use the queue if a worker is available. One more place I found is in the file below, the function |
…ing (PR #2101) Agent-Logs-Url: https://github.com/FlexMeasures/flexmeasures/sessions/2383b9c6-1167-413c-9cf0-e31f2a9db9f5 Co-authored-by: joshuaunity <45713692+joshuaunity@users.noreply.github.com>
Signed-off-by: JDev <45713692+joshuaunity@users.noreply.github.com>
I guess by running |
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
|
I could take on the crowded toasts. I worked on that a bit with Codex in the end. You don't want to get rid of that Success Toast with the link right? (Link could remain internal, indeed). I do like that there is new in-progress Toasts every 2 seconds, which you only see as the counter increases. It tells you that we are still checking ... |
Signed-off-by: F.N. Claessen <claessen@seita.nl>
My first thought was that I like the two ingestion toasts (of which only one is shown in my screenshot) that indicate success for:
In particular, I like that they show "Success", where the forecasting toast just says "Info". But then, I think, isn't it a bit premature to claim success when the job is queued? From a user perspective, the action can only be considered a success when the job is actually finished. The forecasting toasts only show "Success" when the job is finished, so actually I prefer that. After all, you hit the button to get the job done (here: new data in the database), and nothing less should be claimed a success.
For the UI, the "Checking status..." followed by actual status updates (i.e. QUEUED, STARTED, etc.) is probably more suitable than a link to the JSON representation of the job itself. The "Upload received. Processing data in the background" in particular seems redundant. |
|
@nhoening if you have an opinion about how long ingestion jobs should be kept around, let's hear it. :) I think we're just using an RQ default right now (500 seconds), unlike for the forecasting and scheduling queues, which we default to keep around for a full week. |
Shorter, for sure. Maybe a day. |
…m:FlexMeasures/flexmeasures into copilot/move-post-data-logic-to-job-queue
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
I meant actually, that that should probably go, sorry for being unclear. @joshuaunity wrote a JS function |
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
|
Scheduling/forecasting Jobs live a day, their results 7 days: Should we do just the same for ingestion? There might be more (smaller and less interesting) jobs for this. We could simply reuse FLEXMEASURES_JOB_TTL for both right? |
|
@Flix6x here is one thing about those most-recent-only filters: If in the uploaded data we tick the |
Hmm, best to try it. My first thought is that the filter would still not be needed. A good test might be to post data with a 2-hour resolution to a 6-hour sensor, then hit the (1-hour frequency) replay. My hunch is that with the filter we'd see a realization once every 6 hours, and without the filter we'd see an extra two forecasts, 2 and 4 hours into the 6-hour event. |
|
Actually, I shouldn't call them forecasts, but partial realizations. That is, I expect each 6-hour realization to be updated twice. |
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
I'm not sure I get why this answers our question.
How about adding the filter, but only if the |
It has one belief per 2-hour event, and each of those are recorded right at the end of the 2-hour event. If you resample to 6 hours, then 2 hours into that 6-hour event, you have some partial information about the outcome of the event. And 4 hours in, you have some more. That's how you could get 3 beliefs per 6-hour event.
Our replay just has a 1-hour frequency by definition. We just don't have UI support right now to pick a different step.
Let's see the test results first. |
Ill work on this configuration |
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: F.N. Claessen <claessen@seita.nl>
|
I did the test. Here's the difference, exactly as I tried to describe. With most-recent-beliefs-only.mp4Without ( all-beliefs.mp4 |
So I'm out of the loop by now - what is your suggested course of action seeing this? |
|
Before making a decision, I investigated further. The filter was introduced in 513a81e specifically mentioning slow file uploads without it. I verified this issue with a file upload of 1200 rows (2-hour upsampled to 6-hour). So for the sake of UX we should add back the filter. My initial worry was actually that we'd lose support for uploading datasets containing multiple forecasts per event. However, the file upload already did not support that use case, so I consider it out of scope. More context: I envision our time series file import should at some point happily accept our own time series file exports. Follow-ups: I'll re-introduce the filter, fix the failing test and merge. |
Description
ingestionjob queue so large POSTs can be processed asynchronously when a worker is available202 Acceptedresponses with ajob_monitor_urlandjob_idwhen ingestion is queuedFLEXMEASURES_MAX_SENSOR_DATA_INGESTION_BYTESdocumentation/changelog.rstLook & Feel
202 Acceptedwhile ingestion is processed in the background when aningestionworker is availableingestionworker is available, the existing synchronous path still works as a fallbackHow to test
ingestionqueue, for example:flexmeasures jobs run-worker --queue "forecasting|scheduling|ingestion"202 Acceptedquickly with ajob_monitor_url.ingestionworker and confirm the synchronous fallback still works.FLEXMEASURES_MAX_SENSOR_DATA_INGESTION_BYTESand confirm it is rejected with413 Payload Too Large.uv run pytest flexmeasures/api/v3_0/tests/test_sensor_data.pyuv run pytest flexmeasures/api/v3_0/tests/test_sensors_api.pyuv run pytest flexmeasures/data/tests/test_data_ingestion.pyAlso, here is a small script to test the sensor data endpoint:
Further Improvements
Related Items
Sign-off