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

Add SDK docs #4928

Merged
merged 32 commits into from
Sep 15, 2022
Merged

Add SDK docs #4928

merged 32 commits into from
Sep 15, 2022

Conversation

zhiltsov-max
Copy link
Contributor

@zhiltsov-max zhiltsov-max commented Sep 9, 2022

Motivation and context

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.

@@ -0,0 +1,31 @@
---
title: "Integration"
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 like the topic. From my perspective integrations are more about collaboration with external tools (see https://voxel51.com/docs/fiftyone/integrations/index.html). The topic about our REST API, SDK, and CLI. Probably the best option is to just have 3 separate topics: REST API, SDK, CLI. Look at https://docs.labelbox.com/docs as well. I like the documentation structure.

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 can see all the variants in different tools:

  • Labelbox: API & SDK
  • 51: API, CLI
  • Label studio: Integrations (API, SDK, CLI)
  • Superannotate: SDK (+CLI); Integrations (with others + their dataset format)
  • v7: SDK; CLI; API

Probably, I agree about separate sections for SDK and CLI, but not sure about API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed section name to API & SDK. The API & SDK & CLI, API, SDK & CLI variants look clumsy to my taste, and SDK can also suppose CLI by its meaning, so it shouldn't be a big loss.

@zhiltsov-max zhiltsov-max changed the title Add integration docs [WIP] Add integration docs Sep 13, 2022
@zhiltsov-max zhiltsov-max changed the title [WIP] Add integration docs [WIP] Add SDK docs Sep 13, 2022
@zhiltsov-max zhiltsov-max changed the title [WIP] Add SDK docs Add SDK docs Sep 13, 2022
@nmanovic
Copy link
Contributor

/check

@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2022

❌ Some checks failed
📄 See logs here

README.md Outdated
- [Datumaro dataset framework](https://github.com/cvat-ai/datumaro/blob/develop/README.md)
- [Command line interface](https://opencv.github.io/cvat/docs/manual/advanced/cli/)
- [Server API](https://opencv.github.io/cvat/docs/integration/api/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it still a valid link?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you have added the information below. Should we remove these 3 links?

Copy link
Contributor Author

@zhiltsov-max zhiltsov-max Sep 15, 2022

Choose a reason for hiding this comment

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

I like to have these links. Maybe, we could provide a TOC section in the beginning? It will simplify navigation for users, because the current page is quite long. Example from Datumaro:
Screenshot from 2022-09-15 11-18-01
Then, the link will just navigate to the section below.

@@ -14,6 +14,8 @@

from .version import VERSION

SSL_VERIFICATION_ENV_VAR = "SSL_VERIFY"
Copy link
Contributor

Choose a reason for hiding this comment

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

SSL_NO_VERIFY is more or less standard name for the variable (e.g., conda, git use it with a prefix)

Copy link
Contributor Author

@zhiltsov-max zhiltsov-max Sep 15, 2022

Choose a reason for hiding this comment

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

Probably, from popular tools, only these 2 tools use variables at all:

curl: --insecure
wget: --no-check-certificate
pip: install ... --global http.sslVerify false
apt: -o "Acquire::https::Verify-Peer=false" ...
git: GIT_SSL_NO_VERIFY=true git
git: config http.sslVerify "false"
conda: SSL_NO_VERIFY=1 conda ...
npm: config set strict-ssl false
maven: -Dmaven.wagon.http.ssl.insecure=true

Copy link
Contributor Author

@zhiltsov-max zhiltsov-max Sep 15, 2022

Choose a reason for hiding this comment

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

I decided to only use the --insecure arg instead.

@@ -47,6 +49,12 @@ def make_cmdline_parser() -> argparse.ArgumentParser:
description="Perform common operations related to CVAT tasks.\n\n"
)
parser.add_argument("--version", action="version", version=VERSION)
parser.add_argument(
"--ssl-verify",
Copy link
Contributor

Choose a reason for hiding this comment

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

at the same time, the argument name is correct. I don't know why for an env variable and a command line argument the different convention is used.

weight: 29
description: 'Guide to working with CVAT tasks in the command line interface. This section on [GitHub](https://github.com/cvat-ai/cvat/tree/develop/cvat-cli).'
weight: 4
description: ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: ''
description: 'Python command line interface to interact with a CVAT instance'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This description is only displayed as an extra line under the title. I'm not sure this description adds anything to the title

simplify server interaction and provide additional functionality like data validation.

SDK API includes 2 layers:
- Low-level API with REST API wrappers. Located in at `cvat_sdk.api_client`. [Read more](/integration/sdk/lowlevel-api)
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 , the link is broken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed links

a command line tool, browser or a script - all interact with CVAT via HTTP requests and
responses:

![CVAT server interaction image](/images/server_api_interaction.svg)
Copy link
Contributor

Choose a reason for hiding this comment

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

I will prefer to have a console icon instead of a user and a list (python script?). Probably you want to ask Alexander to draw a picture for you after he comes back from vacation.

# Here we will use models instead of a dict
task_data = models.DataRequest(
image_quality=75,
start_frame=2,
Copy link
Contributor

Choose a reason for hiding this comment

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

The parameters look strange for data that contains only 2 files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed


Class | Method | HTTP request | Description
------------ | ------------- | ------------- | -------------
_AuthApi_ | **auth_create_login** | **POST** /api/auth/login |
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 think you want to support the table manually. It should be a reference that is generated automatically.

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 agree, but it is another big task.

```python
from cvat_sdk.api_client import ApiClient, apis

api_client = ApiClient(...)
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 think that we should describe all possible ways to use cvat_sdk.api_client. We need to show in the documentation only the recommended way.

user_model = models.User(...)
```

- About
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 see any value in the list of strings. It should be a generated reference documenation.

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.

Let's merge as is and fix comments (if you agree) in next PRs.

@nmanovic
Copy link
Contributor

/check

@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2022

✔️ All checks completed successfully
📄 See logs here

@zhiltsov-max zhiltsov-max merged commit 68375ec into develop Sep 15, 2022
@bsekachev bsekachev deleted the zm/update-docs branch September 23, 2022 12:04
@nmanovic nmanovic mentioned this pull request Dec 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.

not able to loggin with swagger created client
2 participants