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

No CRDS write #186

Merged
merged 7 commits into from Dec 18, 2023
Merged

No CRDS write #186

merged 7 commits into from Dec 18, 2023

Conversation

TheSkyentist
Copy link
Contributor

@TheSkyentist TheSkyentist commented Dec 9, 2023

As discussed in #182, grizli attempts to write to a CRDS file, which can cause issues on systems with shared CRDS files that are write protected. The proposed solution was to copy over the file to the local CONF.

However this does not work due to a big where the entire file path is .lower()ed, rather than just the filename (and so only works on case-insensitive filesystems). I fix the issue by only .lower()ing the filename, not the entire path (a470bff).

To solve the original issue I perform a check to see if the CRDS file is writeable. If not, the CRDS file if copied to the local CONF directory and the changes are applied there (perhaps this should be the default behaviour). Any subsequent processing can then use the writeable version that is created (6d45ab6).

Finally, in the case where the original CRDS file is updated, we must check if the local CONF version is up to date. If out of date, we redo the copying step (97ff0fc and f49f6ee).

These fixes were developed after working through this with @lboogaard with input from @rameyer and @jdavies-st.

@TheSkyentist TheSkyentist changed the title No crds write No CRDS write Dec 9, 2023
@TheSkyentist TheSkyentist deleted the no-crds-write branch December 11, 2023 10:45
@TheSkyentist TheSkyentist restored the no-crds-write branch December 11, 2023 10:51
@TheSkyentist TheSkyentist reopened this Dec 11, 2023
@gbrammer
Copy link
Owner

gbrammer commented Dec 12, 2023

Could you add a test for the presence of the CRDS_READONLY_CACHE environment variable and also do the thing to write the file in CONF if it's found and True?

@TheSkyentist
Copy link
Contributor Author

Good idea! Now implemented in f9b86c3.

@lboogaard lboogaard mentioned this pull request Dec 13, 2023
@gbrammer gbrammer merged commit cab21d8 into gbrammer:master Dec 18, 2023
5 of 9 checks passed
@gbrammer
Copy link
Owner

Looks good now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants