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

Autouse temporary_config in tests #227

Closed
t-young31 opened this issue Jan 14, 2023 · 3 comments · Fixed by #236
Closed

Autouse temporary_config in tests #227

t-young31 opened this issue Jan 14, 2023 · 3 comments · Fixed by #236
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@t-young31
Copy link
Member

Currently tests can modify the shared global Config class. They should be rewirtten to use the new temporary_config context manager

@t-young31 t-young31 added the enhancement New feature or request label Jan 14, 2023
@t-young31 t-young31 added this to the v1.4.0 milestone Jan 14, 2023
@shoubhikraj
Copy link
Collaborator

@t-young31 I would just like to add here that even with temporary_config context manager, the shared global Config is still modified. The context manager just stores the state of Config when it is called, and restores it with Config.__dict__.update() when it exits.

So things like this will happen:

import autode as ade
from autode import Config

kwds = Config.ORCA.keywords  # gets a reference to Config.ORCA.keywords object

with ade.temporary_config():  # saves Config.__dict__
  kwds.sp.functional = 'B3LYP'  # changes the global config state
  # --calculation--
# when context manager returns, Config.__dict__.update() is called
# which restores the state of previous Config, but that also means
# all values in Config are replaced
assert str(kwds.sp.functional) == 'B3LYP'  # this is True
# because kwds now refers to the old Config.ORCA.keywords object
# and changing kwds no longer changes anything in new Config

So for any mutable types like list or object etc. previous references to them will stop working. For immutable types, taking reference is useless.

@t-young31
Copy link
Member Author

thanks @shoubhikraj – an excellent point. I'm ok with that behaviour/quirk if you are?

@shoubhikraj
Copy link
Collaborator

@t-young31 Yes, I think this behaviour is ok. Fixing it is possible, but would require knowing which item is mutable object, and which is not, and there would be many layers of nested mutable objects - Config, ORCA, keywords, sp. I don't think it would be easy to handle all such cases. Also python does not provide any good way of identifying mutable vs immutable objects.

I would be ok with leaving the behaviour, but I think it might be worth adding an extra page in the examples to show the quirks of Config. Is that fine with you?

@t-young31 t-young31 linked a pull request Jan 22, 2023 that will close this issue
3 tasks
@t-young31 t-young31 self-assigned this Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants