-
Notifications
You must be signed in to change notification settings - Fork 36
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
API endpoint for scheduling asset #1065
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
…a scheduling job Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
…ntext Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
except ValueError as err: | ||
return invalid_flex_config(str(err)) | ||
|
||
# todo: make a 'done job' and pass that job's ID here |
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.
Technically, I guess I now implemented this todo, by putting the last job ID (that of the 'done job') in the response. However, I recall we discussed returning a full list of job IDs, so that the API user can use the existing get_schedule
endpoint to retrieve schedules for individual sensors. In that case, the next step would be to spec such a response.
For example:
{
"schedules": [
{
"sensor": 1,
"schedule": "<uuid>"
},
{
"sensor": 2,
"schedule": "<uuid>"
}
]
}
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.
True, returning a list of schedules would be the way to go. That way, we can reuse the endpoint to get the schedules.
scheduler_specs=scheduler_specs, | ||
requeue=requeue, | ||
job_id=job_id, | ||
enqueue=False, # we enqueue all jobs later in this method |
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'm not too sure about my change here, but I believe I saw duplicate jobs in the queue otherwise.
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.
We are enqueuing them later altogether. We are not checking the function argument, though. This is something to be fixed.
job_status = job.get_status(refresh=True) | ||
|
||
jobs.append(job) | ||
|
||
# with job_status=None, we ensure that only fresh new jobs are enqueued (in the contrary they should be requeued) |
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'm not very clear on what this does.
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.
Me neither... This came from the function create_scheduling_job
. I'm not sure why would a job exist, maybe, if the job id is provided and it coincides with one that was queued?
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
…api/endpoint-for-scheduling-asset # Conflicts: # documentation/changelog.rst
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
…ing-asset' into feature/api/endpoint-for-scheduling-asset Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
…ing-asset' into feature/api/endpoint-for-scheduling-asset
@@ -323,10 +323,10 @@ def create_sequential_scheduling_job( | |||
for job in jobs: | |||
current_app.queues["scheduling"].enqueue_job(job) |
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.
We should probably move the enqueue here..
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 mean doing something like if enqueue: ...
**kwargs, | ||
): | ||
""" | ||
Trigger FlexMeasures to create a schedule for a collection of flexible devices. |
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 trigger can also deal with inflexible devices so this is incomplete.
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 first part is true. However, no schedules are created for the inflexible devices.
{ | ||
"start": "2015-06-02T10:00:00+00:00", | ||
"flex-model": [ | ||
{ |
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 example should be updated with the new schema changes (sensor_flex_model field, etc)
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.
That sensor_flex_model
field is not a field in the public API, but rather a field that gets created when the endpoint validation partly deserializes the flex-model
field. It essentially belongs to a schema that is internal to our platform, used only for the purpose of sequential scheduling. The endpoint logic picks up the still serialized sensor_flex_model
field and passes it to the job kwargs.
} | ||
} | ||
|
||
**Example response** |
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.
TODO
|
||
class SequentialFlexModelSchema(Schema): | ||
sensor = SensorIdField(required=True) | ||
sensor_flex_model = fields.Dict(data_key="sensor-flex-model") |
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.
Couldn't this wrap the single device flex model schema? Either extend it via inheritance or nesting it?
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.
It could, but I don't want to actually deserialize the sensor flex model here, because it needs to be passed to the job kwargs in serialized form. I agree that it's neater, though. I could see if I can do something with telling the endpoint decorator to partially deserialize the schema, i.e. only the sensor field. At first glance it seems possible.
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.
We actually don't have a single device flex model schema. We currently have the ProcessSchedulerFlexModelSchema
and the StorageFlexModelSchema
, and potentially others in the future. Right now the use of fields.Dict
already suggests that the SequentialFlexModelSchema
is agnostic to the underlying types of flex models, so that has its merits, too.
|
||
|
||
class AssetTriggerSchema(Schema): | ||
asset = GenericAssetIdField(data_key="id", status_if_not_found=404) |
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 schema could be used via the CLI so the status_if_not_found
will not make sense there. As an alternative, we can catch the exception in the CLI and display the proper message.
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.
Alternatively, we can avoid this situation by providing an already deserialized asset using the AssetIdField
as a CLI field.
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.
Removing the status_if_not_found=404
here would mean that visiting /assets/<non-existing-id>/schedules/trigger
would return a validation error (422 status?) rather than 404 Not Found.
I would actually be fine with that. Given that we are POSTing a resource (job) rather than GETting a resource (asset) here, the asset
should be seen as part of the POSTed payload to be validated.
Even though at first glance it doesn't seem correct if the asset ID is coming from the path, we have already started to treat the payload more RESTlike by including it in with the JSON data to be validated within the same schema.
In the future, we'd want to move to a RESTful approach to jobs anyway, and then the asset
truly becomes part of the JSON payload.
We might already reconsider the choice of endpoint, but I am wary of increasing the scope of this PR too much.
For now I'll remove the 404 here.
Concerning the CLI, we hardly use schemas there at the moment, as opposed to fields, so maybe it's not that much of a problem?
…the POSTed payload to be validated Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
self.status_if_not_found = status_if_not_found | ||
super().__init__(*args, **kwargs) | ||
|
||
def _deserialize(self, value: int, attr, obj, **kwargs) -> GenericAsset: |
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.
value
arg doesn't have the right type considering that you are using int(value)
to convert the object to int
, is it str
?
Description
Schedule an asset using a flex-model that consists of a list of flex-models for individual power sensors.
Look & Feel
URI:
assets/801/schedules/trigger
Body:
Further Improvements
Work in progress:
Created follow-up issues:
Related Items
Closes #485.