Remove error check in ConfigParser

[ricardoV94]

I can't quite tell why this check was added, but it is causing pymc_experimental tests to fail here: https://github.com/pymc-devs/pymc-experimental/actions/runs/11140715111/job/30960085934?pr=300#step:5:1176

which as best as I could track happens when pytest import pymc (multiple times) and this function is executed: https://github.com/pymc-devs/pymc/blob/45069a966d3e123bf98efbc4f28008e31b243df8/pymc/__init__.py#L27-L46

I don't know why PyMC has to add those specific flags, but it seems like the behavior should be supported? I also don't know what pytest is doing to trigger this error.

[ricardoV94]

CI is failing due to micromamba hanging, also seen in other PRs

[michaelosthege]

This is indeed quite strange. My best guess is this:

add_basic_configvars() executes twice, which should not happen if the module has already been imported.

But there's something about _gcd_import in the logs, and it like garbage-collected import?

Maybe (for some reason) the import pytensor got GCed and then executed again alredy during test discovery?

I'm not 100 % on this, but my feeling is that the error check did it's job and we should instead make sure that this entire block only executes exactly once, even if the import got GCed.

[ricardoV94]

Two questions:

1. How do we handle GC?
2. Is it really a problem to try and re-define a variable that already exists in another instance? Why?

[michaelosthege]

How do we handle GC?

We never cared about GC, and under normal circumstances the import executes just once. No idea what about that pmx PR made a difference.

Is it really a problem to try and re-define a variable that already exists in another instance? Why?

We attach config settings as properties on the PyTensorConfigParser:

pytensor/pytensor/configparser.py

Lines 270 to 271 in 8a6e407

	# the ConfigParam implements __get__/__set__, enabling us to create a property:
	setattr(self.__class__, name, configparam)

This can happen at any point, and from any module. By overwriting an existing parameter property, one will likely break other pieces of code that rely on the first version.

You could argue that PyTensorConfigParser has too many features, but for the advertised functionality the check makes a lot of sense.

[ricardoV94]

I don't think it's related to the PR, but how pytest (perhaps newer version) runs. I could reproduce locally by just running pytest on the tests folder.

I'll double check tomorrow

[ricardoV94]

one will likely break other pieces of code that rely on the first version.

Isn't this just a glorified global state? There shouldn't be multiple versions other than within the temporary context manager of change flags

[michaelosthege]

Isn't this just a glorified global state?

Absolutely

There shouldn't be multiple versions other than within the temporary context manager of change flags

What about all those command line flags then? They are the entry point to the global state aka the currently running process.

[ricardoV94]

Which command flags are you thinking about? I've only ever used env variables or pytensorrc

[michaelosthege]

Yes I mean those. FLOATX, COMPILE_MODE and so on

[ricardoV94]

I still don't get what problem this is trying to avoid. Can you give an example?

[michaelosthege]

config.add(
    "timeout",
    """Timeout in seconds for compilation""",
    IntParam(500, _is_gt_0),
    in_c_key=False,
)

# somewhere else, a config parameter with the same name is added
# the check will catch this and raise the error that the parameter already exists
config.add(
    "timeout",
    """Whether graph rewrites can hit a timeout.""",
    BoolParam(False),
    in_c_key=False,
)

[ricardoV94]

config.add(
    "timeout",
    """Timeout in seconds for compilation""",
    IntParam(500, _is_gt_0),
    in_c_key=False,
)

# somewhere else, a config parameter with the same name is added
# the check will catch this and raise the error that the parameter already exists
config.add(
    "timeout",
    """Whether graph rewrites can hit a timeout.""",
    BoolParam(False),
    in_c_key=False,
)

I'm not worried about that, nobody is adding config keys besides us. Since theano didn't need this check in over a decade either, can we conclude it's a mere theoretical concern not a practical/likely one?

[michaelosthege]

Let's hope for the best then

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.74%. Comparing base (8a6e407) to head (b7beb77).
Report is 115 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1009      +/-   ##
==========================================
- Coverage   81.74%   81.74%   -0.01%     
==========================================
  Files         183      183              
  Lines       47733    47731       -2     
  Branches    11616    11615       -1     
==========================================
- Hits        39020    39018       -2     
- Misses       6518     6520       +2     
+ Partials     2195     2193       -2     

Files with missing lines	Coverage Δ
pytensor/configparser.py	92.53% <ø> (-0.05%)	⬇️

... and 2 files with indirect coverage changes