search

facebookresearch / dynabench

Dynamic Adversarial Benchmarking platform
MIT License
24 stars 16 forks source link

adding check in migration script for local developers who will eventu… #881

Closed ktirumalafb closed 2 years ago

ktirumalafb commented 2 years ago

…ally pull this commit but not have any config for a task

ktirumalafb commented 2 years ago

Awesome, LGTM! Thanks for beating me to this. Did this fix it for you? I would've thought that we'd also have to check for cases where the config json is equal to "{}"

Oh yeah good catch, this did fix it for me I think, but I can add that case too

ktirumalafb commented 2 years ago

Actually I think this case is handled because the first statement that is hit in one iteration of the for loop is for key, value in config_dict.items() which will not iterate over anything if config is empty; then the next statement hit is if "metadata" in config_dict: which will also be skipped if config is empty, so then nothing happens to this task if the config is empty

TristanThrush commented 2 years ago

Actually I think this case is handled because the first statement that is hit in one iteration of the for loop is for key, value in config_dict.items() which will not iterate over anything if config is empty; then the next statement hit is if "metadata" in config_dict: which will also be skipped if config is empty, so then nothing happens to this task if the config is empty

Ah I think you're totally right! Looks ready to squash and merge