Refactor options manager

[connorjward]

Key changes:

• Allow users to set command line options for autogenerated prefixes, this logs a warning because it isn't a safe thing to do but some users want it.
• Don't overwrite options set on the command line, and don't drop them after first use.

Closes #37

The MFE in the issue is fixed by this:

from firedrake import *
import pytest


@pytest.mark.parametrize("options_prefix", (None, "", "pippo_"))
def test_change_command_line(options_prefix):
    mesh = UnitSquareMesh(2, 2)
    V = FunctionSpace(mesh, "CG", 1)

    u = Function(V)
    v = TestFunction(V)

    F = inner(u, v) * dx - conj(v) * dx

    problem = NonlinearVariationalProblem(F, u)
    
    # suppose users pass options by command line or via environment variable
    # here we mimic this by inserting the option directly in PETSc database
    # PETSc behavior: -some_option_whatever equivalent to -some_option_<n>_whatever for all n integers
    opts = PETSc.Options(options_prefix if options_prefix is not None else "firedrake_")
    opts["ksp_type"] = "minres"
    solver = NonlinearVariationalSolver(problem, solver_parameters={"ksp_type": "cg"}, options_prefix=options_prefix)
    opts_not_removed = "ksp_type" in opts and opts["ksp_type"] == "minres"
    # remove our option so that it won't conflict with all other tests
    del opts["ksp_type"]
    assert solver.snes.ksp.getType() == "minres" and opts_not_removed

[connorjward]

@JHopeCollins this is only a rough first attempt but I want to check with you on the approach. The main thing I'm doing is moving the options logic into the inserted_options context manager/method instead of __init__. I think that this makes sense because users should be allowed to noodle with PETSc.Options() themselves. Currently the options are baked in when we construct the OptionsManager.

[connorjward]

Here's an example demonstrating what I believe to be quite unintuitive behaviour. I totally get why this isn't recommended practice, but I also think that we should always try and do the intuitive thing.

@pytest.mark.skipnopetsc4py
@pytest.mark.parametrize("options_prefix", (None, "", "custom_"))
def test_commandline_options_change(caplog, options_prefix):
    from petsc4py import PETSc

    options = PETSc.Options()
    om = petsctools.OptionsManager(
        {"opt1": "value1"}, options_prefix=options_prefix
    )

    with om.inserted_options():
        assert options[f"{om.options_prefix}opt1"] == "value1"

    # Put some options in the database after the options manager is created
    options[f"{om.options_prefix}opt1"] = "value2"

    with om.inserted_options():
        # NOTE: not 'value2' because we use the frozen value from when we first
        # made the options manager
        assert options[f"{om.options_prefix}opt1"] == "value1"

    # the original option should be put back (currently it's dropped)
    assert options[f"{om.options_prefix}opt1"] == "value2"

[JHopeCollins]

Your refactor using dict piping made a mistake I made with the default options sets obvious.

The default_options were grabbed from the global options so we shouldn't delete these. For these options we even know for sure that they will be wanted elsewhere!

Suggested change

	# Start building parameters from the defaults so
	# that they will overwritten by any other source.
	parameters = default_options | parameters
	# The parameters to drop from the global options when we leave the
	# inserted_options context. This is everything except for options
	# passed on the command line.
	to_delete = set(parameters.keys())
	# The parameters to drop from the global options when we leave the
	# inserted_options context. This is everything except for options
	# passed on the command line.
	to_delete = set(parameters.keys())
	# Start building parameters from the defaults so
	# that they will overwritten by any other source.
	parameters = default_options | parameters

[connorjward]

Suggested change

	# TODO
	# make sure we warn on usage if prefix is None
	# and the appctx too

[JHopeCollins]

Can we collect all of the unsafe options that were passed and raise a single warning at the end that lists all of them?

[JHopeCollins]

Suggested change

	assert om.options_prefix == true_prefix
	assert om.options_prefix == true_prefix, "The later tests are invalid if these prefixes do not match."

[JHopeCollins]

Please can you add an error message to each assertion describing what failure means?

[JHopeCollins]

Suggested change

	The prefix to look up items in the global options database
	(may be ``None``, in which case the prefix is generated from default_prefix).

[JHopeCollins]

Suggested change

	form "{default_prefix}_{n}", where n is a unique integer. Note that
	because the unique integer is not stable the auto-generated prefix
	for a particular solver may change between python invocations due
	to changes elsewhere in the code.

[JHopeCollins]

Generally good, this looks like it deals with the original problem and also provides more information to users.

1. We will no longer interfere with the global options dictionary or attempt to hide any entries from the auto-prefixed solver.
2. We will warn you if you do use the auto-generated prefix that this is unstable, where previously we would confusingly just silently ignore some options.

I had some small requests then I'm happy to approve. The biggest one is actually fixing a bug from my DefaultOptionsSet PR!