Skip to content

Conversation

@jeffriley
Copy link
Collaborator

@jeffriley jeffriley commented Jul 28, 2021

Enhancement and Defect Repairs:

  • Added code to copy any grid file and/or logfile-definitions file specified to output container.

The motivation is to ensure that all the input data required to reproduce the run is available. The initial conditions/evolutionary parameter values are in the Run_Details file but, prior to this change, any grid file or logfile-definitions file was not included in the output container, so could be inadvertently lost.

I see the COMPAS output container (directory created to contain the output files) as the main repository for output files, and this is definitely true if the user specifies a logfile type of anything other than HDF5 (e.g. CSV, TSV, etc.). If the user does specify a logfile type of HDF5 (probably the most common use case now that HDF5 is available), then a case could be made to consider the HDF5 output file the main repository, and maybe the grid file and logfile-definition file should be included in t he HDF5 file. I don't think users need access to either of those files during post-run analysis (but maybe I'm missing a use case), so the only benefit in adding them to the HDF5 file is to make that file (the HDF5 file) the repository for all data required to reproduce the run (and I think a case could be made for that - if HDF5 is specified, then the only file the user really needs is the HDF5 file, but if anything other than HDF5 file is specified (CSV, TSV, etc.), then the containing directory is required because multiple output files are produced and need to be maintained as a collection).

Bottom line - we could extend this code to add the grid and logfile-definitions file to the HDF5 output file if enough people think that would be useful.

  • Copying a large grid file could take time, and take up much space, so added new program option '--store-input-files' which is TRUE by default. If FALSE, neither the grid file (if specified) nor the logfile-definitions file (if specified) will be copied to the output container (if TRUE, both will be copied (if specified)).

The user should have control over whether the files are copied to the output container.

After this change, any grid file or logfile-definitions file that is specified as a fully-qualified filename (i.e. the string given as the value of the 'grid' variable or 'logfile_definitions' variable in pythonSubmit.py includes both a path and a base filename) will be passed as given to the commandline - no path will be added by pythonSubmit.py (either CWD or compas_input_path_override). The assumption is that if the user specifies a path then that's the path that should be used.

After this change, boolean options set to either "True" or "False" in pythonSubmit.py will be listed on the commandline constructed by pythonSubmit.py: options whose value in pythonSubmit.py is "True" will be listed with no argument (e.g. '--quiet'), whereas options whose value is "False" in pythonSubmit.py will be listed with the argument "False" (e.g. '--quiet False'). (We could list the argument "True" explicitly if that is preferred by most people). Boolean options whose value in pythonSubmit.py is "None" will not be listed on the commandline (and therefore will take the default C++ value).

Close issue #600 when merged
Close issue #601 when merged

- Added code to copy any grid file and/or logfile-definitions file specified to output container.
- Copying a large grid file could take time, and take up much space, so added new program option '--store-input-files' which is TRUE by default.  If FALSE, neither the grid file (if specified) nor the logfile-definitions file (if specified) will be copied to the output container (if TRUE, both will be copied (if specified)).
- Fixed issue #600: changed pythonSubmit.py to treat fully-qualified grid filenames and fully-qualified logfile-definitions filenames correctly (i.e. don't add CWD if the filename is already fully-qualified).
- Fixed issue #601: changed pythonSubmit.py to put all boolean parameters on the commandline, with True or False value.
Enhancement and Defect Repairs:
@jeffriley jeffriley added bug Something isn't working enhancement New feature or request severity_moderate This is a moderately severe bug urgency_moderate This is a moderately urgent issue labels Jul 28, 2021
@jeffriley jeffriley changed the title Copy input fiules (grid, logfile-definitions) to output container + defect repairs Copy input files (grid, logfile-definitions) to output container + defect repairs Jul 28, 2021
Removed extraneous include
@reinhold-willcox
Copy link
Collaborator

@jeffriley I couldn't get it to work. Using a small grid (250 lines) I ran:

COMPAS --grid grid_0.csv 
COMPAS --grid grid_0.csv --store-input-files 
COMPAS --grid grid_0.csv --store-input-files FALSE

I confirmed that the Run_Details for all three correctly show that the parameter was set (or not) and what its value is. But the size of the respective COMPAS_Output.h5 files is the same for all 3, and a quick check of the contents shows that none contains the grid file. Am I doing something wrong?

@jeffriley
Copy link
Collaborator Author

Added code to copy any grid file and/or logfile-definitions file specified to output container.

The files are copied to the output container (by default, the COMPAS_Output directory). Note the text above discussed whether we should put the files inside the HDF5 file, but currently we don't.

@reinhold-willcox
Copy link
Collaborator

Right, ok, I should have read more closely. In that case, yes, it works!

@jeffriley
Copy link
Collaborator Author

Great! So, merge?

@reinhold-willcox reinhold-willcox merged commit c61a6db into dev Aug 2, 2021
@reinhold-willcox reinhold-willcox deleted the store-files branch August 2, 2021 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request severity_moderate This is a moderately severe bug urgency_moderate This is a moderately urgent issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants