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

feat: spin off fallback scheduler from the storage scheduler #846

Merged
merged 19 commits into from Oct 9, 2023

Conversation

victorgarcia98
Copy link
Contributor

@victorgarcia98 victorgarcia98 commented Sep 13, 2023

Description

This PR spins the fallback mechanism off the StorageScheduler and handles an infeasible schedule by creating a new Job that triggers the fallback scheduler.

TODO

Related Items


  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on code under GPL or other license that is incompatible with FlexMeasures

Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
@victorgarcia98 victorgarcia98 marked this pull request as ready for review September 14, 2023 10:47
Signed-off-by: F.N. Claessen <felix@seita.nl>
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

I mostly have minor comments, besides the imo unnecessary chaining restriction and possibly a small but consequential bug: I suspect the fallback scheduler is accidentally using the DataSource of its parent scheduler to save the data (which would undo the primary reason for splitting off the fallback scheduler in the first place). I suggest we explicitly test whether the expected DataSource is present in the database after the fallback scheduler ran.

Very nice test writing btw!

flexmeasures/data/models/planning/tests/test_solver.py Outdated Show resolved Hide resolved
flexmeasures/data/models/planning/__init__.py Outdated Show resolved Hide resolved
flexmeasures/data/models/planning/storage.py Outdated Show resolved Hide resolved
flexmeasures/data/models/planning/storage.py Outdated Show resolved Hide resolved
flexmeasures/data/models/planning/storage.py Show resolved Hide resolved
flexmeasures/data/services/scheduling.py Outdated Show resolved Hide resolved
flexmeasures/data/services/scheduling.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/tests/test_sensor_schedules.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/tests/test_sensor_schedules.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/tests/test_sensor_schedules.py Outdated Show resolved Hide resolved
@Flix6x
Copy link
Contributor

Flix6x commented Sep 15, 2023

(I fixed your mypy issue.)

Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

Very nice. I only have very minor comments left. And if you can add a changelog entry, too, I'd be happy to sign off on this feature.

We can then decide to merge after releasing 0.16, or alternatively we create a release branch for 0.16. The reason why I want to keep this out of 0.16 is because we need to provision clients with the means to adapt to these changes, which you already acknowledge in your main PR description:

  • Explain behavior in the docs
  • Adapt the FM client

I suggest both of these should be follow-up issues.

flexmeasures/data/services/scheduling.py Outdated Show resolved Hide resolved
flexmeasures/data/services/scheduling.py Outdated Show resolved Hide resolved
flexmeasures/data/services/scheduling.py Outdated Show resolved Hide resolved
flexmeasures/data/services/scheduling.py Outdated Show resolved Hide resolved
flexmeasures/data/tests/test_scheduling_jobs.py Outdated Show resolved Hide resolved
@victorgarcia98
Copy link
Contributor Author

documentation/changelog.rst Outdated Show resolved Hide resolved
Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
victorgarcia98 and others added 3 commits September 22, 2023 11:11
Signed-off-by: Victor <victor@seita.nl>
Fix my own typo.

Signed-off-by: Felix Claessen <30658763+Flix6x@users.noreply.github.com>
Move changelog entry from 0.16 to 0.17.

Signed-off-by: Felix Claessen <30658763+Flix6x@users.noreply.github.com>
Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
@Flix6x
Copy link
Contributor

Flix6x commented Oct 7, 2023

876ed2c seems not to belong to this PR.

@victorgarcia98 victorgarcia98 merged commit 5487bbc into main Oct 9, 2023
7 of 8 checks passed
@victorgarcia98 victorgarcia98 deleted the feature/planning/separate-fallback branch October 9, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants