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

SDK layer 2 - cover RC1 usecases #4813

Merged
merged 151 commits into from
Aug 26, 2022
Merged

SDK layer 2 - cover RC1 usecases #4813

merged 151 commits into from
Aug 26, 2022

Conversation

zhiltsov-max
Copy link
Contributor

@zhiltsov-max zhiltsov-max commented Aug 22, 2022

Motivation and context

  • Common:
    • Removed test directories from bandit config
  • Server / API:
    • Fixed API schema for input and output parameters of CRUD, dataset, annotations and backup model endpoints
    • Allowed OpenApiTypes elements in PolymorphicProxySerializer
      • If OpenApiTypes.NONE is present, nullable: true is inserted into the schema
    • Fixed API schema for WriteOnce fields in serializers
    • Removed PUT method on the root model endpoints:
      • /api/tasks/{id}
      • /api/jobs/{id}
      • /api/projects/{id}
      • /api/issues/{id}
      • /api/comments/{id}
      • /api/cloudstorages/{id}
    • LabeledData fields marked optional
    • Issue update methods now don't support message parameter, which was supposed to be write_once. Added an error message in case this field is supplied.
    • Removed invalid pagination options from the children object list endpoints:
      • /api/task/{id}/jobs
      • /api/jobs/{id}/issues
      • /api/issues/{id}/comments
  • Tests
    • rest_api/test_issues now use SDK
    • Added /api/auth/ tests in REST API tests
    • Fixed failing case of task creation, added test
  • SDK
    • Added layer 2 wrappers for:
      • issues
      • comments
      • jobs
      • users
      • missing task methods
      • projects

How has this been tested?

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.

@zhiltsov-max zhiltsov-max changed the title SDK layer 2 - cover RC1 usecases [WIP] SDK layer 2 - cover RC1 usecases Aug 23, 2022
@zhiltsov-max zhiltsov-max changed the title [WIP] SDK layer 2 - cover RC1 usecases SDK layer 2 - cover RC1 usecases Aug 23, 2022
This was referenced Aug 23, 2022
@@ -51,6 +51,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Image search in cloud storage (<https://github.com/cvat-ai/cvat/pull/8>)
- Reset password functionality (<https://github.com/cvat-ai/cvat/pull/52>)
- Creating task with cloud storage data (<https://github.com/cvat-ai/cvat/pull/116>)
- Show empty tasks (<https://github.com/cvat-ai/cvat/pull/100>)
Copy link
Contributor

Choose a reason for hiding this comment

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

@zhiltsov-max , I'm not sure why we have the line in the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure either. Probably, we need to review changelog links.

@@ -66,7 +65,7 @@ def tasks_create(

def tasks_delete(self, task_ids: Sequence[int]) -> None:
"""Delete a list of tasks, ignoring those which don't exist."""
self.client.delete_tasks(task_ids=task_ids)
self.client.tasks.remove_by_ids(task_ids=task_ids)
Copy link
Contributor

Choose a reason for hiding this comment

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

@zhiltsov-max , I don't know all use cases, but probably tasks.delete(task_ids) is better. First argument can be array of ids or array of tasks. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its almost just a copy-paste from what CLI had. I can't say I prefer one way over another. Both work well and both are convertible to each other. I can say that your variant is more focused on manual typing, while the other is better for passing a container without the need of unpacking. I'm not sure if delete(id1, id2, id3) is more frequent pattern in the client code than delete(ids).

@@ -80,11 +79,11 @@ def tasks_frames(
Download the requested frame numbers for a task and save images as
task_<ID>_frame_<FRAME>.jpg.
"""
self.client.retrieve_task(task_id=task_id).download_frames(
self.client.tasks.retrieve(obj_id=task_id).download_frames(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if it is possible or not, but it will be great if the first argument is ID. Thus we can just write tasks.retrieve(task_id).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I got the question. It is ID. Did you mean rename id to id?

README.md
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you put setup.py and README.md into .gitignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are also generated automatically. We decided to remove such files. Moreover, setup.py is meaningless without REST client code.

stdout = self.run_cli("import", str(fxt_backup_file))

task_id = int(stdout.split()[-1])
assert task_id
assert task_id != fxt_new_task.id
assert self.client.retrieve_task(task_id).size == fxt_new_task.size
assert self.client.tasks.retrieve(task_id).size == fxt_new_task.size
Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I see that simple approach with the first argument exists.

Copy link
Contributor

@nmanovic nmanovic left a comment

Choose a reason for hiding this comment

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

@zhiltsov-max , let's merge as is. We need documentation. Probably it is the next highest priority after we merge your next patch.

@nmanovic nmanovic merged commit 53697ec into develop Aug 26, 2022
@nmanovic nmanovic deleted the zm/sdk-l2-me branch August 26, 2022 18:29
@nmanovic nmanovic mentioned this pull request Sep 12, 2022
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.

4 participants