Skip to content

reserve and generate callbacks#540

Open
jdcourcol wants to merge 4 commits intoopenbraininstitute:mainfrom
jdcourcol:reservation
Open

reserve and generate callbacks#540
jdcourcol wants to merge 4 commits intoopenbraininstitute:mainfrom
jdcourcol:reservation

Conversation

@jdcourcol
Copy link
Copy Markdown
Contributor

Implementation reservation and accounting callbacks as in https://github.com/openbraininstitute/launch-system/blob/main/docs/overview.md#task-launching

Activity is created after the reservation though.

@jdcourcol
Copy link
Copy Markdown
Contributor Author

This is what I mentioned this morning.

),
)
def task_launch_endpoint(
async def task_launch_endpoint(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I doubt we can do async here given that the clients and the rest of the code is sync. Isn't there a sync accounting factory to use?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you're right. that is true for my previous PR . I think that indeed there is a sync factory for accounting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I move to sync factory.
There is a cancel reservation if submit job raises an exception now.

Comment on lines +341 to +344
"config": {
"url": failure_callback_url,
"method": "POST",
},
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should this be made consistent with the accounting_success_callback_url below? Like splitting it into failure_callback_url and failure_payload, since currently I have the parameters directly encoded into the URL.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, I think @eleftherioszisis proposal is to have the parameters in "params" indeed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done with the new commit

@chr-pok
Copy link
Copy Markdown
Collaborator

chr-pok commented Jan 26, 2026

Maybe it is not the scope of this PR, but I just realized that my example notebook examples/A_service_and_entitycore/test_endpoints/test_task_launch_endpoint.ipynb is broken due to the latest changes in the endpoints. Would be great if we could make it work again in this or another PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants