Repository navigation
fix(folderparser): take the split from directory names before substrings - #576
Open
BarsamErshadi wants to merge 2 commits into
Open
BarsamErshadi wants to merge 2 commits into
BarsamErshadi wants to merge 2 commits into
Conversation
_decide_split looked for "valid", "train" and "test" anywhere in the lowercased relative path, so a class folder or filename could override the split directory. In a classification dataset, test/train/1.jpg was uploaded to train and train/invalid_ticket.jpg to valid, and unsplit/latest.jpg went to test because of "latest". Nothing warned about it, and the split= argument can't repair a mixed dataset. Take the topmost directory whose whole name is a split (test/, validation/, train_set/), so validation/train/ stays valid while a wrapper like test-project-1/train/ stays train. Otherwise fall back to a match at the start of a word, in the old valid, train, test order, treating a change of case as a word start, so valid_001.jpg, my_test_set/, myTest/ and VOCtest/ keep working and latest/ and invalid/ stop matching. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7 tasks done
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
When
upload_datasetuploads a directory image by image, it infers each image's split in_decide_split, which looks forvalid,trainandtestanywhere in the lowercased relative path, including class folders and the filename. So a class folder or filename can silently override the real split directory: with a classification class calledtrain, images undertest/train/are uploaded to train. Thesplit=argument forces one split for the whole upload, so it can't repair a mixed dataset. Zip uploads (a.zippath oruse_zip_upload=True) return before this code runs, so this PR doesn't affect them.With this change,
_decide_splitworks in two steps:train,training,valid,validation,testortesting, optionally followed byset,dataorimages(valid_set,train-data). Sotest/train/is test andvalidation/train/is valid.valid_001.jpg,my_test_set/,myTest/andVOCtest/keep working, whilelatest/andinvalid/stop matching. The default is still train.A folder that only contains a split word, like
test-project-1/ortest_dataset/, doesn't count in step 1. That keepstest-project-1/train/andtest-project-1/valid/apart instead of sending both to test.Minimal repro:
Before:
After:
I also called
Workspace.upload_datasetwith a stubbed project, on a folder withtest/train/1.jpg,test/bus/1.jpg,validation/train/1.jpgandtraining/test/1.jpg. The split passed toupload_imagefor/test/train/1.jpgwastrainbefore andtestafter. The other three kept the splits they had before:test,validandtrain.Related Issue(s): None that I found. Open PRs #575, #490 and #421 also touch
folderparser.py, but none of their hunks touch_decide_split.Type of Change
The split of some images does change, which is the point of the fix. Additional Context has a table of what changed in a sample of paths, including the changes that could be regressions.
Testing
Test details:
I added seven tests to
tests/util/test_folderparser.py. They cover:validation/test-project-1/not flattening the splitslatest,invalid,contest)myTest,VOCtest_06-Nov-2007, but notLATEST)validation/Six of them fail on the unfixed code. The fallback test passes both before and after, because it guards behaviour that has to stay the same.
{file: split}for all 8 datasets intests/datasets(72 images) before and after. The two dumps are byte-identical.tests/util: 158 tests, OK.unittestdiscovery overtests/, with non-loopback connections blocked): 1285 tests, 1 skipped, 1 error. The error istests.cli.test_train_handler.TestTrainStart.test_start_with_all_options, which tries a real outbound HTTPS connection. It errors the same way with this fix removed, so it is caused by my network block rather than this change. I did not run it with network access.folderparser.pyalso passed.unittest, which is whatCONTRIBUTING.mddescribes. I ran everything on macOS only and did not test against the real Roboflow API.Checklist
Additional Context
These are the behaviour changes for you to judge. I computed the old and new splits for 76 sample paths, and 32 changed. All 32 are in this table. It's a sample, so other paths can change too.
test/train/a.jpg,test/valid/a.jpg,dataset/test/train/a.jpgtrain/valid/a.jpg,train/valid_001.jpg,train/invalid_ticket.jpgtest/train_001.jpgtest_set/train/a.jpg,test_set/valid/a.jpgtesting/train/a.jpg,testing/train_001.jpgtrain_test_split/test/a.jpginvalid/a.jpg,INVALID/a.jpg,Invalid/a.jpglatest/a.jpg,LATEST/a.jpg,Latest/a.jpg,contest/a.jpgbacktest/a.jpgmytest/a.jpg,xtest/a.jpg,MYTEST/a.jpgmyvalid/a.jpgtrainvalid/a.jpgtesttrain/a.jpgtesting/valid/a.jpgtraining/valid/a.jpgtest/fixtures/train/a.jpgtraining_stuff/test/a.jpg,train_v2/test/a.jpg,train2017/test/a.jpgmytest/,xtest/andMYTEST/now land in train. That rule is what stopslatestandinvalidfrom matching. Separated and mixed-case forms still work:my_test,my-test,myTest,imagesValidandVOCtest_06-Nov-2007. A dataset that relies on all-lowercase run-together names would change silently.laTest/andinValid/still match, as they did before.train_v2/test/andtrain2017/test/go to test, which is wrong iftestis a class. The same rule is what keepstest-project-1/train/in train, and a per-image rule can't tell those two apart.testing/valid/is test andtraining/valid/is train. The path is relative to the uploaded folder, so this only matters when that wrapper is inside the upload.validation/train/a.jpgandvalidation/test/a.jpgstay valid, andtraining/test/a.jpgstays train.my_test_set/train/a.jpgandtest-project-1/train/a.jpgstay train.val/images/b.jpgstill goes to train, and so doesval2017/train/a.jpg. I left treatingvalas a split name for you to decide.