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

Refactoring dataset upload #312

Merged

Conversation

lrosemberg
Copy link

@lrosemberg lrosemberg commented Aug 20, 2024

Description

In PR #266, the way it was implemented changes the way the CLI displays the output, and also causes an error when trying to parse the response from upload_image.

In addition, some methods like project.single_upload had too many responsibilities, which made it difficult to handle exceptions correctly.

That said, what does this PR do?

  • Shifts the responsibility for parsing the error correctly to the rfapi layer
  • Makes only the rfapi layer throw exceptions
  • Creates new methods project.upload_image and project.save_annotation, these start to be used inside project.single_upload and workflow.upload_dataset, isolating a good part of the code and keeping the functions with single responsibility
  • Fixes the CLI output
  • Fixes the bug introduced in @266 that fails to parse some image upload responses (more information here: fix: raise UploadErrors for images and annotations #266 (comment))
  • Creates some test cases to validate project.upload

How has this change been tested, please provide a testcase or example of how you tested the change?

  • Created some tests in the code to validate the project.upload
  • Downloaded a dataset, edited a annotation to fail, and uploaded using the CLI, results below:

CleanShot 2024-08-19 at 23 50 31@2x

CleanShot 2024-08-20 at 00 07 22@2x

@lrosemberg lrosemberg marked this pull request as ready for review August 20, 2024 03:28
@lrosemberg
Copy link
Author

@tonylampada I believe that the best way to solve this is to create exceptions for each type of error, making them be thrown from the rfapi layer, so that project.single_upload "breaks" and does not fail silently.

To be able to capture errors correctly in the CLI, I divided the responsibilities of project.single_upload into project.upload_image and project.save_annotation, so that I can reuse them both within project.single_upload and within workspace.upload_dataset, keeping the CLI outputs the way they were before.

@tonylampada tonylampada self-requested a review August 20, 2024 13:05
Copy link
Collaborator

@tonylampada tonylampada left a comment

Choose a reason for hiding this comment

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

FANTASTIC.
Thank you @lrosemberg

@tonylampada tonylampada merged commit 86cd5bf into roboflow:main Aug 20, 2024
6 checks passed
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.

2 participants