Create function to set user config

[roshanrajeev]

Summary

Fixes #5879

I've created a function that lets the user set the user config programmatically.

Before this function, users had to create a config file in the default location(~/.qiskit/settings.conf) or create the config file and add it in the QISKIT_SETTINGS env variable. But now using the set_config function, users can either set the default config or create multiple config files and use it from Qiskit API itself.

Details and comments

Users can import the function set_config(key, value, section (optional), file (optional)) from qiskit.user_config. Configuration (key-value pair) is added to the file passed to the function. If None is passed, it is added to default config file (~/.qiskit/settings.conf)

Users can create multiple config files using this function or write the configuration to default config file. To use the config files, QISKIT_SETTINGS env variable has to be set to the config file path. Otherwise, the default config file will be used.

Below snapshot creates and writes config to default and 2 other config files. These config files are used by setting the QISKIT_SETTINGS env variable.

[CLAassistant]

All committers have signed the CLA.

[1ucian0]

Thanks for this awesome first contribution! Do you mind writing a release note?

reno new set_config

[mtreinish]

Thanks for pushing this, it's a very good start. I had a couple of comments inline.

The other thing I'm wondering is if we want this to be just a general config parser wrapper or we should do validation on the input? I'm personally divided on this I think having input validation (ie checking the key and value are supported by qiskit on the reading side) would make this more useful so people know if they're writing a config file that's valid. But, on the other hand we left the config open ended (and a single section) so that other projects built on terra could expand it. For example, one potential use case we were looking at is adding an aer section in qiskit-aer for setting defaults (like max memory) when running simulations.

Just think out loud, maybe we should have a check if the section is None or 'default' and then do validation on that input and otherwise we let the user do what they want. What do you think?

[mtreinish]

I would also add an optional filename kwarg here too, just in case a user wants to write a new config file outside of what's configured.

[1ucian0]

Does setting QISKIT_SETTINGS cover this use-case?

[mtreinish]

I was thinking more if you want to create a new file separate the one currently loaded. QISKIT_SETTINGS only controls where the path to the file currently being used. So you could use this to adjust the path, but if you wanted to write to a different path from the file currently loaded this wouldn't work unless you did something like:

path_backup = os.get_environ('QISKIT_SETTINGS')
os.environ['QISKIT_SETTINGS'] = '/new/path'
qiskit.user_config.set_config('circuit_drawer', 'latex')
os.environ['QISKIT_SETTINGS'] = path_backup

which seems a bit clunky.

[roshanrajeev]

@mtreinish Is the idea to create the function set_config with filename kwarg to be able to create multiple config files and use QISKIT_SETTINGS as a switch between config files?

[nonhermitian]

I think this needs a way of showing users all the options and what their current values are.

[1ucian0]

Suggested change

	sec_name = section if section is not None else 'default'
	sec_name = 'default' if section is None else section

[1ucian0]

I think this needs a way of showing users all the options and what their current values are.

I agree. Maybe in a following up PR?

[nonhermitian]

It would also be nice to distinguish session variable changes vs those I want as default going forward.

[mtreinish]

It would also be nice to distinguish session variable changes vs those I want as default going forward.

This PR (and the config file in general) are about changing the defaults in the file from inside the qiskit api without having to hand edit the .ini file. There is no distinction between session defaults and defaults otherwise in terms of the user config (unless you have multiple config files and use the env vars to switch between tthem). The user config file is just to enable users to override the baked in defaults (like circuit.draw() defaulting to text). These are can always be overridden with a kwarg but that's not sticky for a session. Or am I misunderstanding something?

[nonhermitian]

Yeah, no I am confused

[roshanrajeev]

@mtreinish Yeah, the set_config function adds config to QISKIT_SETTINGS file, or if not set, to the default config file. So the idea is to add QISKIT_SETTINGS to set config for the current session and without one to set the default configuration.

[1ucian0]

You can re-raise the exception:

Suggested change

	print("Unable to load the config file {}. Error: '{}'".format(
	filename, str(ex)))
	print("Provide a valid file path and make sure "
	"the file or directory has read/write access.")
	raise exceptions.QiskitUserConfigError("Unable to load the config file {}. Error: '{}'".format(filename, str(ex)))

[mtreinish]

Thanks for updating, this is getting closer. A few inline comments and questions. The other thing which I think would be good is to have some unit tests added to https://github.com/Qiskit/qiskit-terra/blob/master/test/python/test_user_config.py testing that this modifies a file as expected. For example, something that would be good to test is that extra sections are preserved when changing default (based on my reading of the code it is) so we can assert and validate that behavior moving forward.

[mtreinish]

Can we name this filename or file_pathinstead of file just to make it clear that it's not a file (or file-like) object but actually a path.

[mtreinish]

Is this for validation? We should have a comment here about the purpose of this. Although if it is for validation shouldn't we be using the filename input if it's present?

[1ucian0]

The previous line sets user_config.filename, which is the file that read_config_file reads.

[mtreinish]

this was actually the whole block, not just line 200. The issue is that filename is set in L198 to os.getenv('QISKIT_SETTINGS', DEFAULT_FILENAME) which means if the users passes in a filename the validation won't be run on it, instead it will be validating the loaded file for the session not the new one.

[roshanrajeev]

But it will be temporary if we set the config from the file user passes.
__init__.py calls a get_config function, which sets the config file to QISKIT_SETTINGS env or default config file.

def get_config():
    filename = os.getenv('QISKIT_SETTINGS', DEFAULT_FILENAME)
    user_config = UserConfig(filename)
    user_config.read_config_file()
    return user_config.settings

So next time there is an import qiskit statement, the config file is reset. This config change might confuse the user right?

[mtreinish]

This is just for validation right? There is no expectation that this is changing the loaded config file. Right now we do no validation if the user specifies an different output path then the configured file. If we're going to validate the output we should do it for all cases. For example right now if you do something like:

from qiskit.user_config import set_config

# Generate a new config file
set_config('transpile_optimization_level', '42', file_path='new_file.ini')

it will work fine which is not desireable because when I go to use new_file.ini later it will fail to load.

However if you do:

from qiskit.user_config import set_config

# Generate a new config file
set_config('transpile_optimization_level', '42')

it will correctly fail. We need to be validating the generated file in all cases.

[mtreinish]

Shouldn't this be:

Suggested change

	filename = file or DEFAULT_FILENAME
	filename = file or os.getenv('QISKIT_SETTINGS', DEFAULT_FILENAME)

so we either write to the specified filename or to the configured settings file.

[mtreinish]

This is getting closer, some more comments inline

[mtreinish]

Using a set will be faster (since a list will traverse to do a search). Not that this section is performance critical or that this will ever be a bottleneck, but it's easy to fix:

Suggested change

	valid_config = ['circuit_drawer',
	'circuit_mpl_style',
	'circuit_mpl_style_path',
	'transpile_optimization_level',
	'suppress_packaging_warnings',
	'parallel',
	'num_processes']
	valid_config = {'circuit_drawer',
	'circuit_mpl_style',
	'circuit_mpl_style_path',
	'transpile_optimization_level',
	'suppress_packaging_warnings',
	'parallel',
	'num_processes'}

[mtreinish]

This is just for validation right? There is no expectation that this is changing the loaded config file. Right now we do no validation if the user specifies an different output path then the configured file. If we're going to validate the output we should do it for all cases. For example right now if you do something like:

from qiskit.user_config import set_config

# Generate a new config file
set_config('transpile_optimization_level', '42', file_path='new_file.ini')

it will work fine which is not desireable because when I go to use new_file.ini later it will fail to load.

However if you do:

from qiskit.user_config import set_config

# Generate a new config file
set_config('transpile_optimization_level', '42')

it will correctly fail. We need to be validating the generated file in all cases.

[mtreinish]

This option doesn't exist anymore, it was removed in relatively recently in #5619

[mtreinish]

I would probably say in the currently configured location since if QISKIT_SETTINGS is set this won't be either either.

[mtreinish]

We should add a note here that any changes to the existing config file will not be reflected in the current session since the config file is parsed at import time.

[mtreinish]

This will cause complaints since several of the values are ints or bools and it's annoying to have to convert those to strings up front. We should cast for people so you can do something like:

set_config('transpiler_optimization_level', 3)

[javabster]

Hi @roshanrajeev how is this going? Let us know when it's ready for re-review or if you have any questions 😄

[roshanrajeev]

Hi @javabster, I guess this is almost done, I was waiting for it to be reviewd!😅

[mtreinish]

LGTM, thanks for updating everything and I apologize for the delay in getting back to review it after your updates it fell through the cracks in my review list.

The only thing that would be good would be to have a negative test case for when validation fails. Something like a test case that calls: self.assertRaises(set_config("transpile_optimization_level", 6) that validates the correct exception is raised. But it's not worth blocking this over.