search

NVlabs / FPSci

Aim Training Experiments
Other
70 stars 23 forks source link

Check for legal results filename #366

Closed bboudaoud-nv closed 2 years ago

bboudaoud-nv commented 2 years ago

This branch adds a FilePath::isLegalFilename() check on the filename used for the results database. Previously the results could fail to write due to an illegal character in either an experiment description or a user name.

The new check should throw an exception when any invalid filename character is detected in the results filename. It is worth noting this may catch characters that technically legal but not encouraged for file naming.

Merging this PR closes #351

bboudaoud-nv commented 2 years ago

It may be worth noting that G3D also has an option for FilePath::makeLegalFilename() that we could use as an alternative to attempt to automatically correct invalid filenames as opposed to just throwing an exception. The behavior of this method is not documented and I haven't looked into it so I didn't use it ahead of throwing the exception for now.

Update: FilePath::makeLegalFilename() replaces any illegal character(s) with _, and has logic to avoid appending multiple _s in a row. This may be a viable approach to avoid illegal names with things like spaces.

bboudaoud-nv commented 2 years ago

Currently this branch also introduces an issue with descriptions/user names that include spaces. This should be resolved before merging...

bboudaoud-nv commented 2 years ago

For now I've moved to using FilePath::makeLegalFilename() as a quick fix here. This is a work-around for the problem of temporary user names (i.e., "Create a new user") and experiment descriptions with spaces in them triggering a runtime exception.

This may only cause issues when something like 2 user IDs/experiment descriptions only differ by sequential illegal characters in which case they may collide... I think this branch is a fine fix for the issue for now

jspjutNV commented 2 years ago

Actually, it looks like there's a likely bug with an extra + ".db" in there. Checking that before merging.

jspjutNV commented 2 years ago

Actually, it looks like there's a likely bug with an extra + ".db" in there. Checking that before merging.

This should be fixed with 137b0da