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(load): add arrow endpoints #2200

Open
wants to merge 43 commits into
base: dev
Choose a base branch
from

Conversation

TheoPascoli
Copy link
Contributor

No description provided.

@TheoPascoli TheoPascoli marked this pull request as draft October 24, 2024 11:48
@TheoPascoli TheoPascoli changed the title feat: first commit feat(load): add arrow endpoints Oct 24, 2024
@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 4, 2024
antarest/study/web/study_data_blueprint.py Outdated Show resolved Hide resolved
antarest/study/web/study_data_blueprint.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
antarest/study/business/load_management.py Outdated Show resolved Hide resolved
@@ -365,6 +366,7 @@ def __init__(
self.adequacy_patch_manager = AdequacyPatchManager(self.storage_service)
self.advanced_parameters_manager = AdvancedParamsManager(self.storage_service)
self.hydro_manager = HydroManager(self.storage_service)
self.load_manager = LoadManager(self.storage_service)
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally speaking for the other matrices, what is your vision here ? For hydro for instance, we want to add the code inside the HydroManager or do we want to create a specific manager on the side ? I personally prefer the 1st option

Copy link
Member

@sylvlecl sylvlecl left a comment

Choose a reason for hiding this comment

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

First comments

def __init__(self, storage_service: StudyStorageService) -> None:
self.storage_service = storage_service

def get_load_matrix(self, study: Study, area_id: str) -> Response:
Copy link
Member

Choose a reason for hiding this comment

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

This "business" layer should be independent from web considerations, it should return a dataframe instead.

Formatting to arrow should be done in the web layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tags=[APITag.study_data],
summary="Update load series data",
)
def update_load_series(
Copy link
Member

Choose a reason for hiding this comment

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

If we want to support arrow input, I don't think we will be able to use a pydantic model (LoadDTO), because they expect JSON, which cannot contain binary data.

So: if we want the same URL to support at the same time JSON and arrow, we will need to separate it in 2 functions I think, for example discriminated by the "accept-content" header.
In any case, I think the function which will accept arrow input will need to have something else than a pydantic model as its argument (bytes or something else, I did not look into fastapi support for binary input).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually, we decided to only support Arrow format. I'll check to see what is best to handle binary inputs

@TheoPascoli TheoPascoli marked this pull request as ready for review December 5, 2024 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants