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

cvat-sdk: Fix creating tasks with non-local files #5058

Merged
merged 2 commits into from
Oct 7, 2022

Conversation

SpecLad
Copy link
Contributor

@SpecLad SpecLad commented Oct 7, 2022

Forcing the Content-Type for the upload_data API call to multipart/form-data does not work, because the current logic for
converting Python values to parts (ApiClient._convert_body_to_post_params) does not encode them in a way that Django REST Framework can understand (it JSON-encodes each part).

Fortunately, we don't actually need to do that, since when we create a task with non-local files, we don't need to upload any files, and so we can just post the original JSON, so do just that.

I couldn't add a test for the remote image case, because CVAT rejects all URLs with non-public IP addressses. However, I did test this case manually.

Motivation and context

Fixes #4962.

How has this been tested?

SDK unit tests + manual testing for remote files.

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

Copy link
Contributor

@zhiltsov-max zhiltsov-max left a comment

Choose a reason for hiding this comment

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

The implementation looks OK to me. However, I'd like to ask for few other changes:

  1. The tests/share is better be called tests/shared_drive or similar. Otherwise, it looks like it's just a directory with shared data for tests (which is also needed, in perspective)
  2. Probably, we need to add a similar test in the rest_api tests layer, there is no such test yet.
  3. Probably, we should check task creation with more params. Maybe just add more/all params in the existing test, or add a new one.

@SpecLad
Copy link
Contributor Author

SpecLad commented Oct 7, 2022

  1. The tests/share is better be called tests/shared_drive or similar. Otherwise, it looks like it's just a directory with shared data for tests (which is also needed, in perspective)

Sure, I can do that.

  1. Probably, we need to add a similar test in the rest_api tests layer, there is no such test yet.
  2. Probably, we should check task creation with more params. Maybe just add more/all params in the existing test, or add a new one.

I think that goes beyond the scope of this PR. The tests I added are sufficient to make sure that the bugfix works; adding other tests would be a separate task.

This way, it can be reused between tests.
Forcing the `Content-Type` for the `upload_data` API call to
`multipart/form-data` does not work, because the current logic for
converting Python values to parts (`ApiClient._convert_body_to_post_params`)
does not encode them in a way that Django REST Framework can understand (it
JSON-encodes each part).

Fortunately, we don't actually need to do that, since when we create a task
with non-local files, we don't need to upload any files, and so we can just
post the original JSON, so do just that.

I couldn't add a test for the remote image case, because CVAT rejects all URLs
with non-public IP addressses. However, I did test this case manually.
@nmanovic nmanovic merged commit b029bc9 into cvat-ai:develop Oct 7, 2022
@azhavoro
Copy link
Contributor

This PR broke e2e test case_107_connected_file_share

CypressError: `cy.exec('mv cypress/integration/actions_tasks3/assets/case_107/image_case_107_1.png cypress/integration/actions_tasks3/assets/case_107/image_case_107_1.png.bk')` failed because the command exited with a non-zero code.

Information about the failure:
Code: 1

Stderr:
mv: cannot stat 'cypress/integration/actions_tasks3/assets/case_107/image_case_107_1.png': No such file or directory

@SpecLad
Copy link
Contributor Author

SpecLad commented Oct 12, 2022

My bad; didn't see that usage. I'll try to fix it.

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.

CLI create task error with sorting_method
4 participants