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

do not automatically create a global configuration file/dirs when it is not found #3223

Closed
wants to merge 13 commits into from

Conversation

neutrinoceros
Copy link
Member

PR Summary

this is an implementation of the change I proposed in #3045
I wouldn't be surprised that the naive approach breaks a test or two, but maybe it won't. If it does, I hope it will still be easy to fix the places where we currently can't recover from a state where the config file simply doesn't exist, if any.

@neutrinoceros neutrinoceros added proposal Proposals for enhancements, changes, etc enhancement Making something better labels Apr 22, 2021
@neutrinoceros neutrinoceros changed the title do no automatically create a global configuration file/dirs when it is not found do not automatically create a global configuration file/dirs when it is not found Apr 22, 2021
@neutrinoceros
Copy link
Member Author

Ok this turns out a lot more fragile than I anticipated. The following test is very useful in understanding how so:
yt/utilities/tests/test_config.py::TestYTConfigCommands
The underlying issue is that the cli tool yt config set isn't flexible enough to work when no pre-existing configuration is found. I'm not sure I want to invest the time to refactor this properly. In the mean time I'm switching this to draft.

@neutrinoceros neutrinoceros marked this pull request as draft April 22, 2021 11:04
@neutrinoceros neutrinoceros force-pushed the hotfix_3045 branch 2 times, most recently from 25bc742 to 3886e4f Compare April 22, 2021 18:25
… it's missing, fix setup of tests that involve writing a global config file
@neutrinoceros
Copy link
Member Author

I've modified the configuration code so that the config file (and the potentially missing parent dirs) is only created when and if we attempt to write to it (for instance via the CLI yt config set). Should be ok now.

@neutrinoceros neutrinoceros marked this pull request as ready for review April 22, 2021 20:47
@neutrinoceros neutrinoceros marked this pull request as draft April 23, 2021 10:08
@neutrinoceros
Copy link
Member Author

Switching to draft, I need to make sure that yt config get ... works correctly even without a config file

@neutrinoceros
Copy link
Member Author

I believe the config system is much too complicated and hardly maintainable since it's already very hard to change such a tiny "implementation detail". I intend to attempt a refactor of the whole configuration system at some point. If that happens, I'll be using this as resource (some tests I wrote might be still usable then).
Otherwise this has no future and I'll probably close it if I can't find a moment to address the root cause in the following year.

@matthewturk
Copy link
Member

We did just change the config system, so ... let's let it cool a little bit before any refactorings. Before we undertake any refactorings (which I think, formally, are not supposed to have any API changes or differences to end user / public APIs, I think?) we need to identify what we want to accomplish, and I really think we need to start thinking about "How does this improve things?" beyond code clarity. Code clarity is important! Making things simpler is good! But it's also potentially really invasive, and it's also something that can be draining of attention, and a bit overwhelming. So let's talk through before undertaking another big one.

@neutrinoceros
Copy link
Member Author

neutrinoceros commented May 19, 2021

I have two main reasons for wanting to do this. Code clarity isn't one.

1 - Isolation
Right now, the config system isn't isolated, which means that when one runs a simple command from the CLI, the whole package gets imported (and in particular unyt as well, which as we discussed is taking a while).

As a result

yt config set data_dir_path /my/path

takes a few seconds, while what it accomplishes can clearly be done in a mere micro seconds.
Rebuilding the CLI and/or the config system in isolation of the rest of the code would make a tremendous difference in terms of user experience (IMO).

2 - Maintainability
I've been able to identify several bugs in the CLI yt config (other than the poor perf), but I think I can say with confidence than it took more time to try (and fail at) fixing them than it would have taken to rewrite the internals from the bottom up.

Before we undertake any refactorings (which I think, formally, are not supposed to have any API changes or differences to end user / public APIs, I think?)

That's right. I don't intend to break api at any point.

We did just change the config system, so ... let's let it cool a little bit before any refactorings.

Agreed, there's been a lot going on in this part of the code between the 3.6 and the 4.0 releases, so I'm definitely not going to touch it further before the big release.

I'm convinced there's a very clear way to rebuild this that would fix the problems I'm concerned with, but it's not a priority, though I keep bumping into them so I'll attempt something eventually :)

@neutrinoceros
Copy link
Member Author

closing for now, I give up on finding a tidy solution to fix just this problem and maybe I'll propose a clean refactor in the future.

@neutrinoceros neutrinoceros deleted the hotfix_3045 branch June 14, 2021 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better proposal Proposals for enhancements, changes, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants