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

new upload parameter for overwriting annotations #216

Merged

Conversation

npelzmann
Copy link
Contributor

Description

Added a new parameter for the single_upload method to enable overwriting annotations via the API. This has been requested by multiple other users. The option was available via the REST API, but not the python SDK.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

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

Tested upload of image with annotations then deleted part of the annotations manually, and re-uploaded only the annotations.

Any specific deployment considerations

One of the unittests was already broken on the main branch (test_download_returns_dataset in tests.test_version. I did not touch this.

Docs

  • Docs updated? What were the changes: No changes made to docs.

Copy link
Contributor

@SolomonLake SolomonLake left a comment

Choose a reason for hiding this comment

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

Really nice work! Just one comment about supporting backwards compatibility on the single_upload method.

@@ -469,6 +469,7 @@ def single_upload(
image_path=None,
annotation_path=None,
annotation_labelmap=None,
annotation_overwrite=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move to the end of the parameters list so as to keep backwards compatible with previous versions?

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SolomonLake I accidentally closed the issue before actually merging. I think you need to approve a second time. Sorry for the inconvenience.

Copy link
Contributor

Choose a reason for hiding this comment

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

Failed to build due to styling -- running make style in the root directory should fix the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hope the styling checks out now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, i'll merge the PR now!

Copy link
Contributor

@SolomonLake SolomonLake left a comment

Choose a reason for hiding this comment

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

LGTM, after you merge it will be included in the next package release.

@npelzmann npelzmann closed this Jan 10, 2024
@npelzmann npelzmann reopened this Jan 10, 2024
@SolomonLake SolomonLake merged commit 9650fdd into roboflow:main Jan 10, 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