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

RFC: decouple yt.config from initialization #3626

Merged
merged 1 commit into from
May 16, 2022

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Oct 28, 2021

PR Summary

This patch postpones the "live" part of the configuration to the very end of yt.__init__ and disentangle the configuration code a bit. This is achieved by

  • never importing yt.config at the top of any module. Import statements are relocated to inside functions that need to access the global yt.config.ytcfg object. This is in line with the existing practice in the yt.funcs module
  • making yt.config depend on yt.utilities.configure (instead of the other way around). This means a sizeable part of the diff here is simply due to relocating the YTConfig class
  • adding a small callback system in yt.utilities.configure, allowing any module to perform configuration at runtime, but delaying it until yt.config is imported from for the first time (in yt.__init__)

The first commit fixes #3625 while the second fixes #3045

It also has obvious conflicts with #3106, but I think it'd be easier to merge the present PR first.

@neutrinoceros neutrinoceros changed the title ENH: decouple yt.config from initialization RFC: decouple yt.config from initialization Oct 28, 2021
@neutrinoceros neutrinoceros added the refactor improve readability, maintainability, modularity label Oct 28, 2021
@neutrinoceros neutrinoceros force-pushed the decouple_yt_config branch 3 times, most recently from 901d75b to 7eb8f8e Compare October 29, 2021 11:36
@neutrinoceros
Copy link
Member Author

After I managed postponing configuration to the end of yt.init, I was able to fix #3045 trivially

@neutrinoceros neutrinoceros linked an issue Oct 29, 2021 that may be closed by this pull request
@neutrinoceros neutrinoceros force-pushed the decouple_yt_config branch 4 times, most recently from bab71ba to 12631a3 Compare October 29, 2021 13:52
@neutrinoceros neutrinoceros reopened this Nov 19, 2021
@neutrinoceros neutrinoceros marked this pull request as ready for review November 19, 2021 17:58
@neutrinoceros
Copy link
Member Author

pre-commit.ci autofix

@neutrinoceros neutrinoceros force-pushed the decouple_yt_config branch 2 times, most recently from ff9401c to 88f9464 Compare December 6, 2021 22:40
@matthewturk
Copy link
Member

This seems like a good refactor; your third bullet point is intriguing to me though and I'd like to hear if you have anything in mind. If you fix the conflicts I think it's good to go.

@neutrinoceros
Copy link
Member Author

your third bullet point is intriguing to me though and I'd like to hear if you have anything in mind.

I'm not promoting this callback system as a feature-enabling change, it's mostly there to allow fixing the issues I was looking into without breaking backwards compatibility.

If you fix the conflicts I think it's good to go.

solved !

@neutrinoceros
Copy link
Member Author

Forced pushed. Marking as draft until CI passes.

@neutrinoceros neutrinoceros marked this pull request as draft January 7, 2022 15:55
@neutrinoceros neutrinoceros marked this pull request as ready for review January 10, 2022 15:00
@matthewturk
Copy link
Member

@neutrinoceros Do you think it is worthwhile to explore a wrapper function (for our wrapper function!) that manages the imports appropriately? Something like how we do on-demand imports, or something like that. I am slightly anxious about the increased verbosity of adding from yt.config import ytcfg inside all the functions; it's not bad but I think it might discourage using the config system at all.

@neutrinoceros
Copy link
Member Author

I don't have a clear idea how to do that, and I'm convinced that I can come up with a different solution that will not add technical debt :/

@neutrinoceros
Copy link
Member Author

Actually here's an idea to avoid having to nest imports:
the callbacks may be run explicitly by calling a master function living inside yt.config, so it'd be easy to control when that function is called (at the end of yt.__init__). I've tested this locally and can easily update this PR. Would that work for you ?

@matthewturk
Copy link
Member

@neutrinoceros I think that would be great!

@neutrinoceros
Copy link
Member Author

Ok for today I'll just resolve conflicts and switch this back to draft. I'll get back to this.

@neutrinoceros neutrinoceros marked this pull request as draft February 14, 2022 21:35
@neutrinoceros neutrinoceros marked this pull request as ready for review February 15, 2022 14:50
@neutrinoceros
Copy link
Member Author

Switching to draft to give priority to #3831, since these are conflicting but only #3831 needs backporting

@neutrinoceros neutrinoceros marked this pull request as draft March 3, 2022 10:59
@neutrinoceros neutrinoceros marked this pull request as ready for review March 3, 2022 16:39
@matthewturk
Copy link
Member

@neutrinoceros Should we re-up this?

…was initialized

Add a callback system for post-initialization configuration that can be utilize
to spread bits of the configuration throughout the code base but still run them
only in a final stage when importing yt.

Avoid automatically creating a yt configuration file when none is present
@neutrinoceros
Copy link
Member Author

@matthewturk conflict solved

@matthewturk matthewturk enabled auto-merge May 16, 2022 19:51
@neutrinoceros
Copy link
Member Author

@matthewturk did you mean to approve this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor improve readability, maintainability, modularity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: yt.config module is too tightly coupled to yt.__init__ Unnecessary config file creation
2 participants