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

there is no clean way to specify backend-specific args #98

Closed
glennklockwood opened this issue Oct 6, 2018 · 6 comments
Closed

there is no clean way to specify backend-specific args #98

glennklockwood opened this issue Oct 6, 2018 · 6 comments

Comments

@glennklockwood
Copy link
Contributor

This is an issue following the thread on issue #94.

Right now different AIORIs have different methods of exposing backend-specific options. There have been a couple of generalized ways in which users specify these:

  • ior -a HDF5 -- --collectiveMetadata
  • ior -a HDF5 --hdf5.collective_md
  • ior -a HDF5 --custom "hdf5_collective_md=1"
  • ior -a HDF5 --hdf5-collective-md=1

There is the added problem that the current option parser treats anything input via -f as a second-class citizen, which can cause bugs to manifest if the API is specified in the script rather than on the command line.

In such cases, the default API (MPI-IO) would be what gets detected by the time backend->get_options gets called. If the API is then changed to, say, RADOS in the config script, backend->get_options will never get called.

Fixing this issue and bringing command-line options and configfile options at parity might require some major restructuring because the semantics of the command line are different from the config file.

  1. The command line options can be specified in any order (with the exception of -f)
  2. The configfile options must be placed in the correct order with respect to the RUN statements that are to use those configurations

The correct way to handle specification of the backend-specific options would be to immediately do the following (from ParseCommandLine) every time the API option is specified:

        initialTestParams.backend = backend;
        initialTestParams.apiVersion = backend->get_version();

        if (backend->get_options != NULL) {
                option_parse(argc - parsed_options,
                             argv + parsed_options,
                             backend->get_options(),
                             &printhelp);
        }

However doing this means that the backend-specific options would have to first be pulled from the remaining CLI and/or configfile options and passed to the backend-specific option_parse before resuming the parsing the "universal" command-line or configfile options as before.

Unless we want to go down this very complex route (essentially build an entire ordered execution plan in memory before doing anything), we'll have to embed backend-specific options in the API option itself. For example,

api=hdf5=collective_md=1,...

This bundles the backend-specific options with the line that should trigger the parsing of these backend-specific options.

Does this seem ridiculous, or should we plan on writing a much more complicated option parser?

@JulianKunkel
Copy link
Collaborator

Generally, I do believe the way "scripts" are handled should be changed alltogether.
Now that there exists a library style version, the script processing could be externalized, allowing even more options / to handle mdtest as well / concurrent execution of different benchmarks. That was a thought at least and the reason I had not yet done much to deal with the general issue of dealing with backend options inside scripts.

I do agree, though. I'm working to get this fixed changing the parser to support the "plugin.option" syntax which fixes the issue hopefully for the next release.

@JulianKunkel
Copy link
Collaborator

I have updated the parser, see #104.
Now it supports handling all plugins together, e.g.,
./src/ior -a dummy --dummy.delay-xfer=50000 --hdf5.collectiveMetadata
@roblatham00 your thoughts on this? That should fix the issue generally -- I have not yet modularized all options, though, will wait for feedback before finalizing this.

@JulianKunkel
Copy link
Collaborator

Are we good with this, then the pull request can be merged...

@glennklockwood
Copy link
Contributor Author

Sorry, I haven't had a lot of time to look at this. I only have two concerns (which you may have addressed):

  1. Does this maintain backwards compatibility with any existing options? For example, will IO-500 have to be updated?

  2. Does this still support taking input through the script file?

If both are yes, then I'm in favor of merging it in.

@JulianKunkel
Copy link
Collaborator

JulianKunkel commented Oct 19, 2018 via email

@glennklockwood
Copy link
Contributor Author

As long as it doesn't break input scripts, let's merge it in then. But we should keep this issue open until input scripts and CLI options are both in harmony.

If you couldn't tell, I prefer using input scripts over CLI-based job setup, and ensuring that I can continue doing so is a personal priority 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants