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
FLEXMEASURES_MAX_PLANNING_HORIZON can be int #583
Merged
Flix6x
merged 7 commits into
main
from
570-take-resolution-into-account-for-maximum-size-of-planning-window
Jan 17, 2023
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
43af9b6
FLEXMEASURES_MAX_PLANNING_HORIZON can be int
Flix6x 5d458a5
Set default FLEXMEASURES_MAX_PLANNING_HORIZON to the smallest number …
Flix6x 4ab3727
Update documentation of config setting
Flix6x fcc223a
Merge branch 'main' into 570-take-resolution-into-account-for-maximum…
Flix6x 71dec7e
Refactor: util function to get max planning horizon
Flix6x 34980d1
Merge branch 'main' into 570-take-resolution-into-account-for-maximum…
Flix6x 64caa52
Changelog entry
Flix6x File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just thinking about this, and it seems to me that this is a rather large default horizon. This will eat a lot of CPU budget.
To me, ~10% of this is reasonable (a bit over 2 weeks for a 1-hour sensor, a day for a 5-minute sensor)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a thought for the future: this might have to change per account (at first via an attribute, I guess), as this is possibly relevant for billing flex scheduling services.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the default horizon (which is currently set to 2 days), but the default max horizon. I agree that this is a rather large value, but it is what the FlexMeasures scheduler can currently handle without the scheduling jobs timing out after 180 seconds.
I also thought of coupling this PR with the introduction of a config setting to allow FlexMeasures hosts to overwrite python-rq's default job timeout of 180 seconds for the scheduling queue, but I decided to save that for a separate PR. That would be a simple change in
app.py
, something like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, sorry for the oversight. Actually, I would advise we give
FLEXMEASURES_PLANNING_HORIZON
the same treatment (that it can be an int).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that, too, but I didn't find the use case particularly convincing. Is it a worthwhile extra feature, or just resolves an itchy asymmetry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter. But these things matter for user experience. I would expect these two to work the same.
And I believe we are not breaking existing settings here. Now is the cheapest time to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started implementing your request, but it turns out this is probably not true. The
FLEXMEASURES_PLANNING_HORIZON
is used in API versions 1.2 and 1.3, which allow retrieving schedules for multiple sensors in a single API call. That code assumes that the planning horizon is the same for all of these sensors, which with an integerFLEXMEASURES_PLANNING_HORIZON
would no longer be the case. So this would currently require refactoring of already deprecated code.I'd say it is cheaper to implement your request after these API versions are sunset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable, can we make a ticket for version 0.14?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#586