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

56 update code to match new api spec #57

Merged
merged 5 commits into from
Sep 24, 2024

Conversation

bschroeter
Copy link
Contributor

A ridiculous amount of work to address the new API spec. I've been assured that any endpoint changes going forward will be namespaced (i.e. api/v1, api/v2 etc.) to avoid lengthy re-implementation delays.

I took this opportunity to improve docstrings etc. other comments will be written in-situ.

Let me know if anything is unclear.

@bschroeter bschroeter linked an issue Sep 19, 2024 that may be closed by this pull request
@click.command("delete")
@click.argument("output_id")
@click.argument("file_id")
def file_delete(output_id: str, file_id: str):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New deleting method.

cli_file.add_command(file_attach)
cli_file.add_command(file_detach_all)

cli_file.add_command(file_delete)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added for CLI access.

@@ -221,8 +221,8 @@ def logout(self):
def _upload_files_parallel(
self,
files: Union[str, Path, list],
id: str,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

attach_to is renamed to id everywhere now.

@@ -248,16 +248,17 @@ def _upload_files_parallel(
# Do the parallel upload
responses = None
responses = meop.parallelise(
self._upload_file, n, files=files, attach_to=attach_to, progress=progress
self._upload_file, n, filepath=files, id=id, progress=progress
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing the id over to the parallelise method.


return mu.ensure_list(responses)
# return mu.ensure_list(responses)
return responses

def _upload_file(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

_upload_files was capable of uploading multiple files in the one payload, however, this was causing issues with return types (lists vs single objects) so it was simpler to make it the singular case. This is fine as the packet limit on the upload basically prevents uploading multiple files of any meaningful size anyway.

TL;DR - making this single-file made everything a lot simpler for the other methods which all call this.

self, id: str, files: list
) -> Union[dict, requests.Response]:
"""Attach files to a model output.
def delete_file_from_model_output(self, id: str, file_id: str):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New delete method, turns 2 calls into 1

@@ -9,9 +9,10 @@

# Files
FILE_LIST = "modeloutput/{id}/files"
FILE_UPLOAD = "upload"
FILE_UPLOAD = FILE_LIST
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new API endpoints

@@ -1,10 +1,11 @@
"""Test the CLI actions."""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated all of the tests to make things work. Cleaned up docstrings, switched out for fixtures.

@bschroeter bschroeter requested review from a team and ccarouge and removed request for a team September 19, 2024 04:14
Copy link
Contributor

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

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

LGTM. A simple typo-like comment since you were cleaning up the docstrings.

meorg_client/client.py Outdated Show resolved Hide resolved
Co-authored-by: Claire Carouge <[email protected]>
@bschroeter bschroeter merged commit 2176c1e into main Sep 24, 2024
2 checks passed
@bschroeter bschroeter deleted the 56-update-code-to-match-new-api-spec branch September 24, 2024 02:13
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.

Update code to match new API spec
2 participants