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

[DEVX-217] Cleanup Repo #211

Merged
merged 21 commits into from
Nov 22, 2023
Merged

[DEVX-217] Cleanup Repo #211

merged 21 commits into from
Nov 22, 2023

Conversation

sainivedh
Copy link
Contributor

@sainivedh sainivedh commented Nov 15, 2023

What

  • Includes removal and refactor of long pending items for the repo

Delete

  • clarifai.auth as now langchain is updated
  • Dataset Upload Examples and Loaders which is available in clarifai-examples repo
  • Remove split arg in upload_dataset()
  • Remove task arg in upload_dataset()
  • Remove module_dir in upload_dataset

Refactor

  • Rename chunk_size to batch_size in dataset/inputs
  • Rename url_init arg name to url
  • Refactor ClarifaiDataLoader to include task property
  • Refactor classes to labels in all features to be consistent
  • Support multi-labels classification in dataset upload
  • Add DATASET_UPLOAD_TYPES in constants
  • Restructure dataset_loader to more usable dataloader in upload_dataset
  • Set default batch_size to 32 from 128
  • Change text_cls task name to full text_classification
  • Modified loaders
  • Review comments

@sainivedh sainivedh changed the title Devx 217 cleanup repo [DEVX-217] Cleanup Repo Nov 15, 2023
Copy link
Contributor

@isaac-chung isaac-chung left a comment

Choose a reason for hiding this comment

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

Looks good. A few minor comments and questions.

clarifai/client/app.py Outdated Show resolved Hide resolved
clarifai/client/dataset.py Show resolved Hide resolved
clarifai/datasets/upload/base.py Outdated Show resolved Hide resolved
clarifai/datasets/upload/image.py Outdated Show resolved Hide resolved
clarifai/datasets/upload/image.py Outdated Show resolved Hide resolved
clarifai/datasets/upload/examples/README.md Show resolved Hide resolved
@sainivedh sainivedh marked this pull request as ready for review November 21, 2023 15:57
Copy link
Contributor

@isaac-chung isaac-chung left a comment

Choose a reason for hiding this comment

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

Looks good. A few minor comments.

@@ -28,24 +28,24 @@ class App(Lister, BaseClient):
"""App is a class that provides access to Clarifai API endpoints related to App information."""

def __init__(self,
url_init: str = "",
url: str = "",
app_id: str = "",
base_url: str = "https://api.clarifai.com",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Put this url into constants

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 assume you're referring to base_url?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change this in a later PR

| | Segmentation | `coco_segmentation` |
| | Captions | `coco_captions` |
|[xVIEW](http://xviewdataset.org/) | Detection | `xview_detection` |
| [ImageNet](https://www.image-net.org/) | Classification | `imagenet_classification` |
## Contributing Modules
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: To avoid confusion with Clarifai's Modules, maybe we could use the name of the directory.

Suggested change
## Contributing Modules
## Contributing To Loaders

clarifai/datasets/upload/loaders/README.md Show resolved Hide resolved
Comment on lines +45 to +46
# note1: concept_name can be human readable
# note2: concept_id can only be alphanumeric, up to 32 characters, with no special chars except `-` and `_`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: No need to enumerate notes.

Comment on lines +72 to +73
#convert mask to polygons and add to annots
contours, _ = cv2.findContours(mask, cv2.RETR_TREE, cv2.CHAIN_APPROX_SIMPLE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just found this, and is out of scope of this PR. Annotations support masks as base64 images: Annotation -> Data -> Image. How come we are converting them into polygons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that, when we support seg masks?

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 is in todo planning, Need to extend the support for mask uploads.

@sainivedh sainivedh merged commit 68bcbd8 into master Nov 22, 2023
4 checks passed
@sainivedh sainivedh deleted the DEVX-217-cleanup-repo branch November 22, 2023 09:42
@sainivedh sainivedh mentioned this pull request Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants