Skip to content
This repository was archived by the owner on Jul 13, 2026. It is now read-only.

Add FiftyOne datamodule - #209

Merged
lumurillo merged 8 commits into
mainfrom
lumurillo/add-fiftyone-datamodule
Aug 24, 2023
Merged

Add FiftyOne datamodule#209
lumurillo merged 8 commits into
mainfrom
lumurillo/add-fiftyone-datamodule

Conversation

@lumurillo

@lumurillo lumurillo commented Jul 27, 2023

Copy link
Copy Markdown
Contributor

What does this PR do?

Add a new datamodule that loads FiftyOne datasets. This implementation let MART delegate the data handling to FiftyOne, where the user can do data curation tasks.

This data handling would be agnostic to MART, because at the end here is adapted a torchvision dataset.

I set this implementation to be optional. The user can install the fiftyone dependencies by running:

pip install -e .[fiftyone]

Important:

Type of change

Please check all relevant options.

  • Improvement (non-breaking)
  • Bug fix (non-breaking)
  • New feature (non-breaking)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Testing

Please describe the tests that you ran to verify your changes. Consider listing any relevant details of your test configuration.

  • pytest
  • CUDA_VISIBLE_DEVICES=0 python -m mart experiment=CIFAR10_CNN_Adv trainer=gpu trainer.precision=16 reports 70% (21 sec/epoch).
  • CUDA_VISIBLE_DEVICES=0,1 python -m mart experiment=CIFAR10_CNN_Adv trainer=ddp trainer.precision=16 trainer.devices=2 model.optimizer.lr=0.2 trainer.max_steps=2925 datamodule.ims_per_batch=256 datamodule.world_size=2 reports 70% (14 sec/epoch).

Before submitting

  • The title is self-explanatory and the description concisely explains the PR
  • My PR does only one thing, instead of bundling different changes together
  • I list all the breaking changes introduced by this pull request
  • I have commented my code
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have run pre-commit hooks with pre-commit run -a command without errors

Did you have fun?

Make sure you had fun coding 🙃

@lumurillo
lumurillo requested review from dxoigmn and mzweilin July 27, 2023 03:41
@lumurillo lumurillo self-assigned this Jul 27, 2023
@lumurillo lumurillo mentioned this pull request Aug 2, 2023
15 tasks

@mzweilin mzweilin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for bringing the new feature. I was able to install, but I don't have an example to test with.

Comment thread mart/configs/datamodule/fiftyone.yaml Outdated
Comment thread mart/configs/datamodule/fiftyone.yaml Outdated
Comment thread mart/datamodules/fiftyone.py Outdated
Comment thread mart/datamodules/fiftyone.py Outdated
# load FiftyOne dataset
if dataset_name in fo.list_datasets():
self.dataset = fo.load_dataset(dataset_name)
elif dataset_name in foz.list_zoo_datasets():

Copy link
Copy Markdown
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 we want to load zoo datasets here, because it tries to index the dataset every time.

Maybe we should revert the changes, and use the FiftyOne command line to pre-load zoo datasets as datasets. For example, fiftyone zoo datasets load -s validation -n coco-2017-validation coco-2017 loads the validation split of coco-2017 as a dataset named "coco-2017-validation".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, makes sense

@lumurillo
lumurillo requested a review from mzweilin August 10, 2023 23:15
@mzweilin

Copy link
Copy Markdown
Contributor

FiftyOne commands to load (index) datasets.

Use COCO-2017 as an example. Unfortunately, FiftyOne does not support person-keypoints annotations yet.

Download and load zoo datasets

fiftyone zoo datasets load \
coco-2017 \
-s train \
-n coco-2017-instances-train \
-k include_id=true label_types=detections,segmentations

fiftyone zoo datasets load \
coco-2017 \
-s validation \
-n coco-2017-instances-validation \
-k include_id=true label_types=detections,segmentations

Load local datasets

fiftyone datasets create \
--name coco-2017-instances-validation \
--dataset-dir /raid/datasets/coco/ \
--type fiftyone.types.COCODetectionDataset \
--kwargs \
data_path="val2017" \
labels_path=/raid/datasets/coco/annotations/instances_val2017.json \
persistent=true \
include_id=true

Use the FiftyOne datamodule

datamodule:
  train_dataset:
    dataset_name: coco-2017-instances-train
    gt_field: segmentations
  val_dataset:
    dataset_name: coco-2017-instances-validation
    gt_field: segmentations

@lumurillo

Copy link
Copy Markdown
Contributor Author

FiftyOne commands to load (index) datasets.

Use COCO-2017 as an example. Unfortunately, FiftyOne does not support person-keypoints annotations yet.

Download and load zoo datasets

fiftyone zoo datasets load \
coco-2017 \
-s train \
-n coco-2017-instances-train \
-k include_id=true label_types=detections,segmentations

fiftyone zoo datasets load \
coco-2017 \
-s validation \
-n coco-2017-instances-validation \
-k include_id=true label_types=detections,segmentations

Load local datasets

fiftyone datasets create \
--name coco-2017-instances-validation \
--dataset-dir /raid/datasets/coco/ \
--type fiftyone.types.COCODetectionDataset \
--kwargs \
data_path="val2017" \
labels_path=/raid/datasets/coco/annotations/instances_val2017.json \
persistent=true \
include_id=true

Use the FiftyOne datamodule

datamodule:
  train_dataset:
    dataset_name: coco-2017-instances-train
    gt_field: segmentations
  val_dataset:
    dataset_name: coco-2017-instances-validation
    gt_field: segmentations

Is this for the for the example documentation?

@mzweilin mzweilin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@lumurillo
lumurillo force-pushed the lumurillo/add-fiftyone-datamodule branch from 6b718c8 to e31e0cb Compare August 24, 2023 20:28
@lumurillo
lumurillo merged commit e5df863 into main Aug 24, 2023
@lumurillo
lumurillo deleted the lumurillo/add-fiftyone-datamodule branch August 24, 2023 20:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants