UX: warn when env variables are ignored in cache and config directory discovery#17934
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
923e49b to
4f018d5
Compare
|
Have you thought about what should happen if the directory exists but the user doesn't have permissions to access it? |
|
Read access to the cache and config directories should be sufficient as long as one doesn't need to download additional data. So, we could check for read permission and raise an exception if not present. It's probably a good idea to check for write access too (at least for the cache directory), but a warning might be sufficient there. ... but I think these improvements are orthogonal to what I'm doing here, right ? |
|
In this pull request we should think about what should we do to validate the directories. As far as I can tell there are three things that can go wrong:
Is there any failure mode I missed? |
|
Sorry I am late to the discussion. The XDG behavior has been established for like almost 15 years now, there is little benefit to only now throw warning. In my original review of #17640 , I want the new ASTROPY env vars to either auto-create the dir tree or be verbose instead of silently falling back to what XDG does. Because in that case, user already purposely chooses to not use the XDG behavior. So in response to Eero's #17640 (comment) , I am not sure if (2) and (3) should be separate. |
|
API consistency makes things simpler to implement, simpler to test, simpler to document and, most importantly, simpler for the users to understand. Why should we deviate from the general guideline in this case? |
|
Cost / benefit. There is little benefit to break something that no one complained about for almost 15 years. |
|
Is a warning considered "breaking" now ? I think the complexity cost of treating |
Yes. |
|
I don't get it. In what conditions do we expect any user to have these variables set to invalid locations, and it not be a mistake ? |
|
I'd be willing to accept that the users don't get warnings because that is the current behavior (I don't really like the current behavior, but it is what it is). But I cannot agree that the |
Why not? One is specific to astropy and you can be sure that this variable is set because of astropy. The other is a generic variable that users might set for all kinds of reasons and have no idea it actually also affects astropy. |
|
Also, |
|
I agree with @neutrinoceros and @eerovaher above: This is a bug and fixing bugs is exactly what minor releases are for (in fact, that's even what bug fix releases are for, it doesn't even have to be a minor release). To paraphrase what others have said above: There is no way someone sets the defaults for their configuration directories without wanting to set the defaults for their configuration directories. So, if I have a typo and that directory does not exist or the disk is not mounted or something, then I absolutely have to know that - astropy should issue a warning or exception. |
|
I also agree with @neutrinoceros and @eerovaher and now @hamogu above on the principle of adding a warning here. I have not looked at the code or done technical review, but I think that once @eerovaher agrees that this is technically sound then it can be approved for 7.2. |
|
I agree for the config dir, but I think there should be no warning for a non-existent cache directory as long as the location is writable. It's common to clear caches or create them fresh by setting the cache variable to another value. An empty cache shouldn't be worth a warning. |
|
that's a good point. We could create the |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
a423f7e to
0157032
Compare
pllim
left a comment
There was a problem hiding this comment.
I think we're very close now. The logic route LGTM but I feel like the warning stuff could be simplified.
docs/changes/config/17934.bugfix.rst
Outdated
| @@ -0,0 +1,3 @@ | |||
| ``get_config_dir()`` and ``get_cache_dir()`` now emit warnings in all cases | |||
| where the ``XDG_CACHE_HOME`` (resp ``XDG_CONFIG_HOME``) environment variable | |||
There was a problem hiding this comment.
I don't know what "resp" means. Can you please clarify more here?
There was a problem hiding this comment.
whoop, that must be french only, I assumed it was common in english too. It's just short for "respectively". I'll just use the unabbreviated word then
There was a problem hiding this comment.
(FWIW, I found 4 other occurences in the code base, some of which were not from french contributors 😅, but I agree there's no point shortening a word if it's not commonly understood. Thanks !)
|
@pllim I haven't included your suggested simplifications for now, as I anticipate they would lead to further churn in following developments, but if you still think it's worth it, please apply all of them and squash(-merge?) ! |
4d5be99 to
90e8e13
Compare
This comment was marked as resolved.
This comment was marked as resolved.
It is okay. Since you anticipate for the follow-up PR, no need to apply the suggestions. Thanks for the clarifications! |
On it |
90e8e13 to
f953162
Compare
|
rebased and cleaned the branch history ! |
0487ad2 to
74e5561
Compare
astropy/config/paths.py
Outdated
| if linkto.exists() and linkto.resolve() == maindir: | ||
| # no-op | ||
| linkto = None | ||
| elif sys.platform.startswith("win"): | ||
| warn("is not supported on Windows") | ||
| linkto = None | ||
| elif maindir.is_dir() and linkto.resolve() != maindir: | ||
| warn( | ||
| f"the default location, {maindir}, already exists, and takes precedence" | ||
| ) | ||
| linkto = None |
There was a problem hiding this comment.
I think this is more aligned with existing main logic, which skips linkto as long as it exists, no matter what. Here, we can emit extra warnings, but we should not change the main logic.
| if linkto.exists() and linkto.resolve() == maindir: | |
| # no-op | |
| linkto = None | |
| elif sys.platform.startswith("win"): | |
| warn("is not supported on Windows") | |
| linkto = None | |
| elif maindir.is_dir() and linkto.resolve() != maindir: | |
| warn( | |
| f"the default location, {maindir}, already exists, and takes precedence" | |
| ) | |
| linkto = None | |
| if linkto.exists(): | |
| linkto = None | |
| if maindir.is_dir() and linkto.resolve() != maindir: | |
| warn( | |
| f"the default location, {maindir}, already exists, and takes precedence" | |
| ) | |
| elif sys.platform.startswith("win"): | |
| warn("is not supported on Windows") | |
| linkto = None |
For reference, on main:
if (
not sys.platform.startswith("win")
and linkto is not None
and not linkto.exists()
):
linkto.symlink_to(maindir)There was a problem hiding this comment.
Well I can't take this suggestion as is because:
linkto = None
if maindir.is_dir() and linkto.resolve() != maindir:
...would raise an AttributeError: None has not attribute 'resolve'.
Though I take your point. Let me think about it
There was a problem hiding this comment.
Ok I'm now pretty sure that you just discovered yet another bug in my implementation, but this one is worse, in a sense, because I think it shows how I already broke perfect bug-for-bug compat between 7.0 and 7.1. I'm going to add a test case for it and revert to 7.0 behavior.
There was a problem hiding this comment.
(luckily, this should actually make the logic simpler, for once)
There was a problem hiding this comment.
nevermind that, the weird behavior I was looking at was actually already in 6.1, which is before I got involved with it.
There was a problem hiding this comment.
okay so bear with me: at this point in the code, we've already returned something if linkto is either a symlink or a dir, and we warned + discarded it if it was a file. Knowing that it's not None here, we already know that it can't be an existing path, so a branch such as if linkto.exists() will not be reachable at all. I would normally be okay with having redundant checks, since this logic is spread across more than one function, but here I actually think it's better to keep is as "simple" as possible, so my conclusion was to just remove this branch. I hope this makes sense
47eb532 to
7ab9245
Compare
… variables are ignored, and expect warnings to be emitted
9a68306 to
0f0749f
Compare
pllim
left a comment
There was a problem hiding this comment.
This is now at the state where
- it issues the warning it originally wanted to issue
- a few more warnings for some weird combos
- no annoying unexpected warning for common use cases (as far as I could locally test and I did not test on Windows)
While not perfect, we should get this in before we confuse ourselves further. Thank you very much for your patience!
|
And thank you a thousand times over for your unfailing diligence throughout this difficult review. I know we both got very frustrated with it but I'm convinced the end result is much better than what I originally envisioned, and I couldn't have made it without your help. |
Description
This is split from #17640 at @eerovaher's request.
Fix #18436