-
Notifications
You must be signed in to change notification settings - Fork 276
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
BUG: also validate unresolved symlinks (RAMSES) #3786
BUG: also validate unresolved symlinks (RAMSES) #3786
Conversation
…ixes Issue yt-project#3785 The extra keyword in the loader for the RAMSES frontend makes sure that the symlinks are not resolved by the `RAMSESFileSanitizer`. This is particularly helpful if the file structure is actually a set of links to the files, for instance to bypass the fact that `RAMSESFileSanitizer` doesn't deal well with outputs including "groups".
Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution! |
That was quick ! |
That being said I stand by my statement that not having the option to skip link resolving is a bug since it breaks your workflow, which seems absolutely sound to me (sorry for going back and forth on this). I think we could merge this on the condition that it doesn't close #3785, which should only be closed by the correct patch. What do you think ? Note that if you go forward with this patch, you need to take care of linting (pre-commit.ci), see our docs on the matter https://yt-project.org/doc/developing/developing.html#automatically-checking-and-fixing-code-style |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding a couple suggestions on function signatures
One last thought for today: how about naming the flag |
Happy to think about it, but I'm not sure I understand enough of the code to not break anything. In any case, it would take a bit more time, but I can try to have a look (I'll maybe bug @cphyc for help with the Ramses frontend too…)
That would work for me :-)
Thanks for this, and for all the suggestions. Implementing and updating ASAP. |
I only have one comment remaining, after that I'd be inclined to merge this as a bug fix but I'd require a second reviewer because expanding the api could be regarded as a feature and not a bugfix. In my opinion this is a necessary addition to a half-baked feature (auto-resolving everything) which it's too late to take back, which is why I consider it a bugfix. If a second reviewer agrees with this view, we could even ship this with the next bugfix release |
I'm on holiday till next week so if we want this merged ASAP, dont wait for me!
8 Feb 2022 16:55:35 Clément Robert ***@***.***>:
… ***@***.**** approved this pull request.
—
Reply to this email directly, view it on GitHub[#3786 (review)], or unsubscribe[https:/notifications/unsubscribe-auth/ABJJIIYYUHI4QFI57RTKS7LU2E4HNANCNFSM5NYGL3LA].
Triage notifications on the go with GitHub Mobile for iOS[https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675] or Android[https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub].
You are receiving this because you were mentioned. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACgAAAAoCAYAAACM/rhtAAAABHNCSVQICAgIfAhkiAAAABxJREFUWIXtwQEBAAAAgiD/r25IQAEAAAAAAHwaGSgAAaRY3EgAAAAASUVORK5CYII=###24x24:true###][Tracking image][https:/notifications/beacon/ABJJII2MVCY3JQYPZFBVXXTU2E4HNA5CNFSM5NYGL3LKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOGQ5KDGQ.gif]
|
Found myself working on a related problem here #3801, and now I'm thinking we could maybe avoid adding a flag to the API if we instead try to validate the user-input path with both the resolved and the unresolved version. This could work for all known workflows for RAMSES data, and would improve the user experience. @mtrebitsch, what do you think about this ? |
@mtrebitsch Here's a proof of concept patch to implement my previous suggestion diff --git a/yt/frontends/ramses/data_structures.py b/yt/frontends/ramses/data_structures.py
index fc95e5727..85f80cfd5 100644
--- a/yt/frontends/ramses/data_structures.py
+++ b/yt/frontends/ramses/data_structures.py
@@ -1,6 +1,7 @@
import os
import weakref
from collections import defaultdict
+from itertools import product
from pathlib import Path
import numpy as np
@@ -42,15 +43,16 @@ class RAMSESFileSanitizer:
def __init__(self, filename):
# Resolve so that it works with symlinks
- filename = Path(filename).resolve()
-
self.original_filename = filename
+ paths_to_try = (Path(filename), Path(filename).resolve())
self.output_dir = None
self.info_fname = None
- for check_fun in (self.test_with_standard_file, self.test_with_folder_name):
- ok, output_dir, info_fname = check_fun(filename)
+ check_functions = (self.test_with_standard_file, self.test_with_folder_name)
+
+ for path, check_fun in product(paths_to_try, check_functions):
+ ok, output_dir, info_fname = check_fun(path)
if ok:
break |
Thanks @neutrinoceros, I'll try that over the week-end and I'll get back to you. This seems much cleaner than adding new keywords all the time indeed :-) |
One question though (sorry, it hit me when I had just sent the previous comment): is |
As far as I can tell, I think it's okay rework this class so that this attribute better matches its name, and I'm confident that |
Thanks @neutrinoceros, the suggestion is working flawlessly (on my side at least)! I believe it solves the symlink issue, even though the PR's title isn't quite right now. :-) |
Excellent ! I edited the title again. Given my involvement I don't want to merge this myself, but hopefully @cphyc will be happy to review it now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not particularly happy about how clunky this turns out to be but it'd require a refactor for it to be nicer.
Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆 |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
Manually back ported in #3789 |
The extra keyword in the loader for the RAMSES frontend makes sure that the symlinks are not resolved by the
RAMSESFileSanitizer
. This is particularly helpful if the file structure is actually a set of links to the files, for instance to bypass the fact thatRAMSESFileSanitizer
doesn't deal well with outputs including "groups".PR Summary
With yt 4, the RAMSES loader does not recognize the "groups" output structure (
/path/to/output/files/output_XXXXX/group_YYYYY/info_XXXXX.txt
). One relatively easy way to bypass that, in principle, is to do link all the output files in a separate folder without the nested "group" structure (/path/to/linked/files/output_XXXXX/info_XXXXX.txt
). Beyond yt, this can be needed/useful for other analysis tools that are less flexible.However, doing so does not work if the symlinks are resolved to their original location. This PR fixes this by adding an extra keyword
preserve_symlinks
to ensure that said links are preserved by theRAMSESFileSainitizer
.This is
False
by default, so it should not break any code.This partially fixes issue #3785.
PR Checklist