Skip to content
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.

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

Add basic parameter file validation mode. #2600

Merged
merged 3 commits into from Aug 24, 2018

Conversation

jperryhouts
Copy link
Contributor

This idea could definitely be improved upon, but is of useful as is. This PR adds a --validate flag that exits after parsing the parameter file. I typically do the same thing by just starting a model and ctrl-c'ing it after the parameter file is parsed. This is very slightly more straightforward, and possible to automate in a script later on if needed.

@tjhei
Copy link
Member

tjhei commented Aug 9, 2018

Does this really work as is? I don't see the logic to check whether parsing works or not. Would you mind setting the return value in main to tell if validation works?

@jperryhouts
Copy link
Contributor Author

Does this really work as is?

Yeah, but it does it the lazy way. It either hits the QuietException in parse_parameters (made more explicit just now with a fixup to this commit), which gets caught in main and returns 1; or it skips constructing the Simulator, and exits with 0 the usual way at the end of main. It short circuits run_simulator, similar to the way --output-xml and --output-plugin-graph work.

I'm not sure I understood your suggestion. I see two other options to make the code more explicit, and they both involve more changes and possible code duplication than I think this feature really deserves:

  1. Pass along a pointer to a boolean to run_simulator, and then to parse_parameters, which eventually comes back to notify main about the result.
  2. Make a separate function that only validates parameters, and call it from main.

@tjhei
Copy link
Member

tjhei commented Aug 10, 2018

Yeah, but it does it the lazy way.

I see! This is difficult to see by looking at the source. So you are saying that your approach does already return failure in main() because of the quiet exception?

Another option (maybe):

if (validate)
{
ParameterHandler prm;
aspect::Simulator<dim>::declare_parameters(prm);
try
{
  parse_parameters (input_as_string, prm);
}
catch (...)
{
throw QuietException();
}
}

instead of introducing additional args to run_simulator and parse_parameters?

@jperryhouts
Copy link
Contributor Author

That'd work, and be more clear/explicit, but to do it from main I think means duplicating those lines for case dim==2 and dim==3. I think I recall that just passing <dim> isn't enough because the compiler needs to make a finite number of object copies of the Simulator class. That's why I was thinking it seems like too much duplication (especially since those first two lines would already then be duplicated from run_simulator).

Alternatively, that could be its own function, called something like validate_prm.

I'm fine with any of these options; let me know which you prefer.

@tjhei
Copy link
Member

tjhei commented Aug 11, 2018

then be duplicated from run_simulator

well, you could put what I wrote into run_simulator instead? Oh, and could you change the name of the argument of run_simulator to validate_only?

I'm fine with any of these options; let me know which you prefer.

I don't feel strongly about it either. :)

@jperryhouts
Copy link
Contributor Author

👍 -- I put your code in run_simulator. This should make the logic clear.

@tjhei
Copy link
Member

tjhei commented Aug 13, 2018

I like it! Thanks. /run-tests

Copy link
Contributor

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

I like this as well!

source/main.cc Outdated
@@ -511,6 +511,7 @@ void print_help()
<< " -j, --threads (to use multi-threading)\n"
<< " --output-xml (print parameters in xml format to standard output and exit)\n"
<< " --output-plugin-graph (write a representation of all plugins to standard output and exit)\n"
<< " --validate (parse parameter file and exit)\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe make the purpose clear by saying "...and either exit or report parsing failures"?

source/main.cc Outdated
throw aspect::QuietException();
}
if (i_am_proc_0)
std::cout << "Valid parameter file." << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

In isolation (without knowing the code that surrounds it), this text is hard to understand. Is the user asked to provide a valid parameter file? Maybe just say "The provided output file is valid."

@jperryhouts
Copy link
Contributor Author

jperryhouts commented Aug 19, 2018

I changed those interface messages to make them more clear. Should be pretty much good to go now.
Question: Should the 'your parameter file is valid...' message be printed to stdout or stderr?

One thing that's sort of unfortunate about this feature is that it's totally possible to write a valid parameter file that will not run. For example, if you choose 3 compositional fields, but only list two field names, this validation will pass, but your model will crash before starting. From the user's perspective, that's an invalid parameter file. The distinction between that and the more superficial validations addressed in this patch isn't at all obvious.

I added a disclaimer after the successful validation message to that effect, but it might be worth thinking of ways to more thoroughly validate prm's in the future (run the simulator for a time step, but turn off all the solvers or something?). The idea here is to make sure your model won't immediately crash on a cluster, where you might not notice the failure right away.

@tjhei
Copy link
Member

tjhei commented Aug 20, 2018

run the simulator for a time step, but turn off all the solvers or something?

The problem with that is that you might not be able to create/setup the initial mesh on your workstation, resume from a snapshot, etc.. or that it will take a very long time. I think "validation" should be something that runs very quickly.

@jperryhouts
Copy link
Contributor Author

I think "validation" should be something that runs very quickly.

Agreed. I don't really have a solution at the moment, but something to think about in the future.

@bangerth
Copy link
Contributor

All of the checks we currently perform are either in the parse_parameters() function or the constructor of class Simulator. The latter can indeed take quite a long time, and may also run in parallel, so just creating such an object may not be trivial.

source/main.cc Outdated
throw aspect::QuietException();
}
if (i_am_proc_0)
std::cerr << "The provided parameter file is valid.\n\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

std::cerr is not the correct place: that's where errors go, but you print a success message. Can you still change this?

@gassmoeller
Copy link
Member

Looks ready. continuous-integration/jenkins/pr-merge actually succeeded and just did not report the results, and continuous-integration/jenkins/pr-head stumbled over an unrelated issue that was fixed on master already. Thanks!

@gassmoeller gassmoeller merged commit edfa6b7 into geodynamics:master Aug 24, 2018
freddrichards pushed a commit to freddrichards/aspect that referenced this pull request May 20, 2019
…ation

Add basic parameter file validation mode.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants