Skip to content

[fix] automatically regenerate the cache when it doesn't match #72

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.

Sign up for GitHub

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

Open
yjmm10 wants to merge 3 commits into
base: TRAIN
Choose a base branch
from
Open

[fix] automatically regenerate the cache when it doesn't match #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f2ecc2
[fix] automatically regenerate the cache when it doesn't match
Aug 14, 2024
d2c36b1
[fix] short the path of cache file
Aug 14, 2024
86da903
add workflow
Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/deploy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: Deploy Mode Validation & Inference

on:
push:
branches: [main]
branches: [main,TRAIN]
pull_request:
branches: [main]

Expand Down
14 changes: 12 additions & 2 deletions yolo/tools/data_loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ def __init__(self, data_cfg: DataConfig, dataset_cfg: DatasetConfig, phase: str
transforms = [eval(aug)(prob) for aug, prob in augment_cfg.items()]
self.transform = AugmentationComposer(transforms, self.image_size)
self.transform.get_more_data = self.get_more_data
self.data = self.load_data(Path(dataset_cfg.path), phase_name)
self.data_root = Path(dataset_cfg.path)
self.data = self.load_data(self.data_root, phase_name)

def load_data(self, dataset_path: Path, phase_name: str):
"""
Expand All @@ -54,6 +55,14 @@ def load_data(self, dataset_path: Path, phase_name: str):
else:
data = torch.load(cache_path)
logger.info("📦 Loaded {} cache", phase_name)
# Validate cache
if data[0][0].parent == Path("images")/phase_name:
Copy link
Contributor

@Abdul-Mukit Abdul-Mukit Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is not verifying the content of the cache. This is just checking whether the phase_name is the same in the data read from cache.
Assuming we only save the image_id (just the file name without extension), we should be verifying two things:

  1. Lenght of images mentioned in the instances_<phase_name>.json is equals to the number of elements in data.
  2. Is the set of image names (excluding extension) form the json file is equal to the set of image_ids recovered from cache.

Copy link
Author

@yjmm10 yjmm10 Aug 15, 2024

Choose a reason for hiding this comment

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

Regarding the second question, I'd like to make an addition. If a complete check is made to see if the cache is consistent with the json file, when dealing with a relatively large data set, the efficiency is similar to regenerating the cache. At present, I only make a comparison for one of them, and there are still quite a few problems.

Copy link
Contributor

@Abdul-Mukit Abdul-Mukit Aug 15, 2024

Choose a reason for hiding this comment

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

I agree with you. The second part will be costly.

logger.info("✅ Cache validation successful")
else:
logger.warning("⚠️ Cache validation failed, regenerating")
data = self.filter_data(dataset_path, phase_name)
torch.save(data, cache_path)

return data

def filter_data(self, dataset_path: Path, phase_name: str) -> list:
Expand Down Expand Up @@ -100,7 +109,7 @@ def filter_data(self, dataset_path: Path, phase_name: str) -> list:

labels = self.load_valid_labels(image_id, image_seg_annotations)

img_path = images_path / image_name
img_path = Path("images") / phase_name / image_name
data.append((img_path, labels))
Copy link
Contributor

@Abdul-Mukit Abdul-Mukit Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of image_path can we consider using image_id as the key?
That is instead do this:
data.append((image_id, labels)) where image_id = Path(image_name).stem
This should meet the requirement of this MR that we can move around dataset randomly.
This also makes the code a bit more predictable. I think it is more intuitive that a cache has keys that are just file names and not the information about train / test split. In txt based YOLO datasets the only thing common among image and label.txt is the common name without the extension. I think it is fitting that the cache should have that in common too. Furthermore, you already have the phase name in the file name train.cache and val.cache. No need to include phase_name again inside the *.cache files.

The current check for cache is just checking whether the train/test split are same as phase_name: if data[0][0].parent == Path("images")/phase_name:. This will no longer be needed. Please also see my comment about this line inside the load_data function.

Copy link
Author

@yjmm10 yjmm10 Aug 15, 2024

Choose a reason for hiding this comment

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

It's a good idea that solved the input info and the question of validation.

valid_inputs += 1
logger.info("Recorded {}/{} valid inputs", valid_inputs, len(images_list))
Expand Down Expand Up @@ -133,6 +142,7 @@ def load_valid_labels(self, label_path: str, seg_data_one_img: list) -> Union[Te

def get_data(self, idx):
img_path, bboxes = self.data[idx]
img_path = self.data_root / Path(img_path)
img = Image.open(img_path).convert("RGB")
Copy link
Contributor

@Abdul-Mukit Abdul-Mukit Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the current proposed solution, we are splitting the image_path in filter_data then we are completing it back in this function. Such distribution makes the code less predictable and difficult to grasp unless the developers know the code base from the heart. I think we should come up with a better solution.

Copy link
Author

@yjmm10 yjmm10 Aug 15, 2024

Choose a reason for hiding this comment

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

I wonder if it's possible to achieve a similar effect as in yolov8, leaving these paths to be specified by the users. The existing code is overly strict in its requirements for the data format.

Copy link
Author

@yjmm10 yjmm10 Aug 15, 2024

Choose a reason for hiding this comment

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

When open image, it's the same that get the iamge path from image_id.

Copy link
Contributor

@Abdul-Mukit Abdul-Mukit Aug 15, 2024

Choose a reason for hiding this comment

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

Not sure, if I fully got your point. If you are suggesting that using image_id (image file name minus the extension) would make things easier, I agree.

Copy link
Contributor

@Abdul-Mukit Abdul-Mukit Aug 15, 2024

Choose a reason for hiding this comment

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

Maybe setting the key to the actual image name with file extension is better?
Saw your comment: #72 (comment)
We already have available to us the dataset_path, phase_name variable. "images" folder name string is common in both coco and yolo datasets. That means we can reconstruct the original path of the image easily if we use the image file name with extension as the key.
image_path = dataset_path / "Images" / phase_name / image_file_name

Extions will help resolve .jpg/.png ambiguity.

return img, bboxes, img_path

Expand Down