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

Per-user defaults for xAH_run (aka a dotfile) #1303

Merged
merged 14 commits into from
Mar 18, 2019
Merged

Per-user defaults for xAH_run (aka a dotfile) #1303

merged 14 commits into from
Mar 18, 2019

Conversation

kkrizka
Copy link
Contributor

@kkrizka kkrizka commented Jan 10, 2019

To run on my institution's cluster, I need to specify a lot of options (ie: queue, setup commands...) to xAH_run.py. This is not very practical if I want to run one-off tests.

This PR solves this by adding a dotfile (~/.xah) that is read by xAH_run.py at start and overrides any defaults passed to the argument parser. It is an INI file with a section per sub-command and a general section for the general options. The keys are the options being set.

As an example, my .xah looks as follows:

[slurm]
optBatchSharedFileSystem=1
optBatchWait=1
optSlurmRunTime=5:00:00
optSlurmExtraConfigLines=#SBATCH --qos=shared --tasks-per-node=1 --constraint=haswell --image=centos:centos7 --export=NONE
optSlurmWrapperExec=export LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:/global/project/projectdirs/atlas/scripts/extra_libs_180822; hostname; shifter --module=cvmfs /bin/bash \

@kratsg
Copy link
Contributor

kratsg commented Jan 10, 2019

don't move the import ROOT because it's how we can build the documentation.

@kkrizka
Copy link
Contributor Author

kkrizka commented Jan 11, 2019

Yup, fixed.

Any comments on the interface? If not, I'll start adding this to the other xAH_run arguments.

scripts/xAH_run.py Outdated Show resolved Hide resolved
@kratsg
Copy link
Contributor

kratsg commented Jan 11, 2019

@kkrizka , I've been thinking a little about this, and I want to take the opportunity to make this easier for you, actually. As it's been pretty clear, more especially so in this PR, the configurations available (their types, the default values, etc...) should be maintained in some particular list that you can directly access or use to automatically generate the dot-file config as well.

Let me submit a PR to rearrange some of this for you, so that this is a bit easier to deal with.


On a second note, I would encourage you to use the exact name of the argument in the config file as well, instead of using a different name, e.g. optSlurmAccount instead of Account. This makes grep'ing much easier for users to do.

@kratsg
Copy link
Contributor

kratsg commented Jan 11, 2019

When #1304 is merged in, you should be able to do something like this

config.add_section('slurm')
for optName,optConfig in xAODAnaHelpers.cli_options.drivers_slurm.iteritems():
  config.set('slurm', optName, optConfig.get('default'))

 config.read(userconfigpath)

# update the cli_options based on the read-in config file
for optName in xAODAnaHelpers.cli_options.drivers_slurm.keys():
  xAODAnaHelpers.cli_options.drivers_slurm[optName]['default'] = config.get('slurm', optName)
...
...
...
xAODAnaHelpers.utils.register_on_parser(xAODAnaHelpers.cli_options.drivers_slurm, slurm)

and then in portions when we build the argument parser, it should see the updated defaults. Let me know if this is a lot more preferable for your needs.

@kkrizka
Copy link
Contributor Author

kkrizka commented Jan 11, 2019

I prefer the dotfile. It is a standard way to configure applications on Linux.

@kratsg
Copy link
Contributor

kratsg commented Jan 11, 2019

I prefer the dotfile. It is a standard way to configure applications on Linux.

This still uses the dotfile.

@kkrizka
Copy link
Contributor Author

kkrizka commented Jan 11, 2019

Ok, I don't see any difference by moving all of the options to a separate file then. So go ahead.

@kratsg
Copy link
Contributor

kratsg commented Jan 11, 2019

Ok, I don't see any difference by moving all of the options to a separate file then. So go ahead.

This way, you're not hard-coding the config.set() options twice -- once for argument parser, and one for config parser.

@kratsg
Copy link
Contributor

kratsg commented Jan 12, 2019

Merged in my changes.

kratsg
kratsg previously requested changes Feb 15, 2019
python/cli_options.py Outdated Show resolved Hide resolved
scripts/xAH_run.py Outdated Show resolved Hide resolved
scripts/xAH_run.py Outdated Show resolved Hide resolved
@kratsg
Copy link
Contributor

kratsg commented Feb 15, 2019

Also, any thoughts on using the existing dotfiles from asetup -- and adding in configurations there -- instead of creating another dotfile .xAH? I suppose at the moment, there's no harm -- but one can imagine this would be useful for others to use as well in different frameworks and then we start seeing hundreds of different dotfiles...

@kkrizka
Copy link
Contributor Author

kkrizka commented Feb 15, 2019

The configuration is very specific to xAH. I don't see a reason to mix its configuration with asetup, which is a separate application.

@kratsg
Copy link
Contributor

kratsg commented Feb 15, 2019

Additional, how do we feel about using something like https://github.com/theskumar/python-dotenv? We can package it up with xAH as part of a CMake installation (and it's relatively small) and might be perfect for something like this? If it's too much, that's also fine.

@kratsg
Copy link
Contributor

kratsg commented Feb 15, 2019

The configuration is very specific to xAH. I don't see a reason to mix its configuration with asetup, which is a separate application.

Right, so the reason I ask is -- for example -- pyproject.toml is becoming (in python3) a centralized, sort-of dotfile for various python packages like flake8 and pytest -- as well as to put setup.py specific things in it as well. One could imagine the same thing here, where xAH plays nicely with other frameworks that depend on xAH like the Xhh folks (so it's less confusing for them to have a ... .atlas or .analysis file with all configurations -- than a .xah).

Also I might request that no matter what we pick, we keep the filename lowercase.

@kratsg
Copy link
Contributor

kratsg commented Feb 15, 2019

Can we move the parsing of dotfile into utils, and the updating of cli_options into utils as well? Then I think I'm happy with this. Main reason I ask is so that other people who depend on xAH (Xhh) can take advantage of the nice utility functions for parsing/updating.

@kratsg
Copy link
Contributor

kratsg commented Feb 15, 2019

And I think one last thought, and maybe a nice thing. I can imagine that you might want to have multiple .xah files -- one in $HOME/.xah and one in $HOME/analysis_wrapper/.xah. E.G. $HOME/.xah contains global settings specific to the machine, while the local one would have settings specific to the analysis. This still follows standard precedence of local overriding $HOME if they share the same keys. This would be, presumably, as simple as

home_dotfile.update(local_dotfile)

before updating anything else. Or just adding another line, so that the local dotfile updates all defaults last.

@kkrizka
Copy link
Contributor Author

kkrizka commented Feb 15, 2019

I've moved the dotfile loading into a function in utils. That way if someone requests analysis-specific dotfiles, this can be added.

python/utils.py Outdated Show resolved Hide resolved
python/utils.py Outdated Show resolved Hide resolved
docs/UsingUs.rst Outdated Show resolved Hide resolved
@kkrizka kkrizka dismissed kratsg’s stale review February 24, 2019 19:36

This have changed.

@kkrizka
Copy link
Contributor Author

kkrizka commented Feb 24, 2019

This PR does not cause #1316 , but the error does highlight an issue with the current proposal. There are certain "common" driver options that I don't want to have the same defaults for. For example, I want optBatchDriver=True for the Slurm driver (local batch, mostly for making histograms that I want automatically merged at the end) and I don't want it for grid (long jobs, make ntuples that I don't want downloaded/merged).

The PR has been changed back to use INI files, with section per sub-command and general for the general options. The documentation is also updated.

I'm going to keep it as WIP for a day or two to further test the latest changes.

@kratsg
Copy link
Contributor

kratsg commented Feb 26, 2019

This is fine. Remove the WIP when you're happy and ping me.

@kkrizka kkrizka changed the title WIP: Per-user defaults for xAH_run (aka a dotfile) Per-user defaults for xAH_run (aka a dotfile) Mar 8, 2019
@kkrizka
Copy link
Contributor Author

kkrizka commented Mar 8, 2019

I've been running with this for a while and I'm happy. So I think it can be merged.

@kratsg kratsg merged commit 36081a2 into master Mar 18, 2019
@kratsg kratsg deleted the xAHrc branch March 18, 2019 20:42
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

Successfully merging this pull request may close these issues.

2 participants