fix: Fix XRootD storage plugin at least for file access #8
Conversation
MattMonk
changed the title
| xrootd_retry = retry( | ||
| tries=3, delay=3, backoff=2, logger=get_logger(), fail_callback=_raise_fatal_error | ||
| ) |
There was a problem hiding this comment.
Why using a special retry decorator here and not the one provided by snakemake-interface-storage-plugin?
There was a problem hiding this comment.
This was just because when I was testing it I would sometimes find that when it failed it would keep retrying on errors that should not really (imo) be retried on like access permissions problems. I figured maybe instead I could have a list of "veto" error codes that should not be re-tried and just fail straight away (thinking things like out of memory on the host side etc.)
I ended up re-defining the retry decorator here because I figured it was maybe the easiest way to get the custom callback function in to throw the "unretry-able" errors straight away instead of doing the usual retries but this extra decorator was a bit of an afterthought so happy to change/remove it!
There was a problem hiding this comment.
I see. This is a good idea and we can keep it like this for now. Maybe we can reintegrate it with the plugin interface at some point.
WalkthroughThe recent updates introduce a Snakemake storage plugin for the XRootD protocol, enhancing data operations in computational workflows. Key improvements include robust error handling, an integrated retry mechanism for transient errors, and flexible URL management. Although the plugin currently supports only file-level operations, these enhancements significantly increase reliability and usability, positioning it as a valuable tool for scientific computing environments. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Workflow
participant XRootDProvider
participant ErrorHandler
User->>Workflow: Submit request for file operation
Workflow->>XRootDProvider: Process request
XRootDProvider->>ErrorHandler: Check for errors
alt No errors
XRootDProvider-->>Workflow: Return file operation result
else Errors encountered
XRootDProvider->>ErrorHandler: Log error
ErrorHandler->>XRootDProvider: Retry operation
XRootDProvider-->>Workflow: Return failure message
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range comments (1)
snakemake_storage_plugin_xrootd/__init__.py (1)
Line range hint
403-411:
Implementlist_candidate_matchesor remove the method.The
list_candidate_matchesmethod is marked with a TODO but is not implemented. Either implement it or remove it if not needed.Do you want me to help implement this method or open a GitHub issue to track this task?
Tools
Ruff
277-277: Missing type annotation for
selfin method(ANN101)
282-282: Missing type annotation for
selfin method(ANN101)
288-288: Missing return type annotation for public function
cleanupAdd return type annotation:
None(ANN201)
288-288: Missing type annotation for
selfin method(ANN101)
292-292: Unnecessary
passstatementRemove unnecessary
pass(PIE790)
295-295: Missing type annotation for
selfin method(ANN101)
298-298: Missing type annotation for
selfin method(ANN101)
298-298: Missing type annotation for function argument
query(ANN001)
315-315: Missing type annotation for
selfin method(ANN101)
325-325: Missing type annotation for
selfin method(ANN101)
335-335: Missing return type annotation for public function
retrieve_objectAdd return type annotation:
None(ANN201)
335-335: Missing type annotation for
selfin method(ANN101)
360-360: Missing return type annotation for private function
_makedirsAdd return type annotation:
None(ANN202)
360-360: Missing type annotation for
selfin method(ANN101)
362-362: Unnecessary parentheses on raised exception
Remove unnecessary parentheses
(RSE102)
371-371: Missing return type annotation for public function
store_objectAdd return type annotation:
None(ANN201)
371-371: Missing type annotation for
selfin method(ANN101)
381-381: Trailing comma missing
Add trailing comma
(COM812)
389-389: Missing return type annotation for public function
removeAdd return type annotation:
None(ANN201)
389-389: Missing type annotation for
selfin method(ANN101)
405-405: Missing type annotation for
selfin method(ANN101)
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
Files selected for processing (2)
- docs/intro.md (1 hunks)
- snakemake_storage_plugin_xrootd/init.py (5 hunks)
Additional context used
Path-based instructions (1)
snakemake_storage_plugin_xrootd/__init__.py (1)
Pattern
**/*.py: Do not suggest to add trailing commas.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Markdownlint
docs/intro.md
1-1: null
First line in a file should be a top-level heading(MD041, first-line-heading, first-line-h1)
Ruff
snakemake_storage_plugin_xrootd/__init__.py
34-34: Unnecessary
passstatementRemove unnecessary
pass(PIE790)
37-37: Missing return type annotation for private function
_raise_fatal_errorAdd return type annotation:
None(ANN202)
45-45: Trailing comma missing
Add trailing comma
(COM812)
94-94: Missing return type annotation for special method
__post_init__(ANN204)
94-94: Missing type annotation for
selfin method(ANN101)
119-119: Missing return type annotation for private function
_check_statusAdd return type annotation:
None(ANN202)
119-119: Missing type annotation for
selfin method(ANN101)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
123-123: Unnecessary
elseafterraisestatement(RET506)
124-124: Replace aliased errors with
OSErrorReplace
IOErrorwith builtinOSError(UP024)
124-124: Avoid specifying long messages outside the exception class
(TRY003)
127-127: Missing type annotation for
clsin classmethod(ANN102)
145-145: Missing type annotation for
selfin method(ANN101)
145-145: Unused method argument:
query(ARG002)
145-145: Unused method argument:
operation(ARG002)
145-145: Dynamically typed expressions (typing.Any) are disallowed in
rate_limiter_key(ANN401)
154-154: Missing type annotation for
selfin method(ANN101)
159-159: Missing type annotation for
selfin method(ANN101)
168-168: Unnecessary
elseafterreturnstatement(RET505)
180-180: Missing type annotation for
selfin method(ANN101)
187-190: Use ternary operator
user_pass = f"{user}:{password}@" if password != "" else f"{user}@"instead ofif-else-blockReplace
if-else-block withuser_pass = f"{user}:{password}@" if password != "" else f"{user}@"(SIM108)
193-193: Replace aliased errors with
OSErrorReplace
IOErrorwith builtinOSError(UP024)
193-195: Avoid specifying long messages outside the exception class
(TRY003)
194-194: Trailing comma missing
Add trailing comma
(COM812)
208-208: Replace aliased errors with
OSErrorReplace
IOErrorwith builtinOSError(UP024)
208-211: Avoid specifying long messages outside the exception class
(TRY003)
210-210: Trailing comma missing
Add trailing comma
(COM812)
212-212: Unnecessary
elseafterraisestatement(RET506)
213-213: Replace aliased errors with
OSErrorReplace
IOErrorwith builtinOSError(UP024)
213-215: Avoid specifying long messages outside the exception class
(TRY003)
214-214: Trailing comma missing
Add trailing comma
(COM812)
228-228: Missing type annotation for
clsin classmethod(ANN102)
240-240: Unnecessary
elseafterreturnstatement(RET505)
255-255: Missing return type annotation for special method
__post_init__(ANN204)
255-255: Missing type annotation for
selfin method(ANN101)
282-282: Missing type annotation for
selfin method(ANN101)
288-288: Missing return type annotation for public function
cleanupAdd return type annotation:
None(ANN201)
288-288: Missing type annotation for
selfin method(ANN101)
292-292: Unnecessary
passstatementRemove unnecessary
pass(PIE790)
295-295: Missing type annotation for
selfin method(ANN101)
298-298: Missing type annotation for
selfin method(ANN101)
298-298: Missing type annotation for function argument
query(ANN001)
315-315: Missing type annotation for
selfin method(ANN101)
325-325: Missing type annotation for
selfin method(ANN101)
335-335: Missing return type annotation for public function
retrieve_objectAdd return type annotation:
None(ANN201)
335-335: Missing type annotation for
selfin method(ANN101)
360-360: Missing return type annotation for private function
_makedirsAdd return type annotation:
None(ANN202)
360-360: Missing type annotation for
selfin method(ANN101)
362-362: Unnecessary parentheses on raised exception
Remove unnecessary parentheses
(RSE102)
371-371: Missing return type annotation for public function
store_objectAdd return type annotation:
None(ANN201)
371-371: Missing type annotation for
selfin method(ANN101)
381-381: Trailing comma missing
Add trailing comma
(COM812)
389-389: Missing return type annotation for public function
removeAdd return type annotation:
None(ANN201)
389-389: Missing type annotation for
selfin method(ANN101)
Additional comments not posted (2)
snakemake_storage_plugin_xrootd/__init__.py (2)
83-87: Ensureurl_decoratoris correctly documented and tested.The
url_decoratoris a new feature that allows for URL modification. Ensure this is well-documented and tested to avoid unexpected behavior.
294-310: Ensure the retry decorator is applied consistently.The
@xrootd_retrydecorator should be consistently applied to methods that can benefit from retry logic, such asexistsand_exists.Verification successful
Consistent Application of
@xrootd_retryDecorator VerifiedThe
@xrootd_retrydecorator is consistently applied to methods interacting with the XRootD system, except forretrieve_object, which is commented out. This might be intentional. Please review if this exception is appropriate.
- Methods with
@xrootd_retry:exists,mtime,size,_makedirs,store_object,remove- Commented out:
retrieve_objectScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistent application of the `@xrootd_retry` decorator. # Test: Search for the usage of `@xrootd_retry` in the codebase. rg --type python '@xrootd_retry'Length of output: 63
Script:
#!/bin/bash # Description: Verify the consistent application of the `@xrootd_retry` decorator. # Find all Python files and search for `@xrootd_retry` within them. fd --extension py --exec rg '@xrootd_retry' {}Length of output: 176
Script:
#!/bin/bash # Description: Capture surrounding lines of each `@xrootd_retry` usage to identify decorated methods. # Find all Python files and search for `@xrootd_retry`, including context lines. fd --extension py --exec rg --context 3 '@xrootd_retry' {}Length of output: 1457
Tools
Ruff
295-295: Missing type annotation for
selfin method(ANN101)
298-298: Missing type annotation for
selfin method(ANN101)
298-298: Missing type annotation for function argument
query(ANN001)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- snakemake_storage_plugin_xrootd/init.py (5 hunks)
Additional context used
Path-based instructions (1)
snakemake_storage_plugin_xrootd/__init__.py (1)
Pattern
**/*.py: Do not suggest to add trailing commas.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Ruff
snakemake_storage_plugin_xrootd/__init__.py
34-34: Unnecessary
passstatementRemove unnecessary
pass(PIE790)
37-37: Missing return type annotation for private function
_raise_fatal_errorAdd return type annotation:
None(ANN202)
45-45: Trailing comma missing
Add trailing comma
(COM812)
94-94: Missing return type annotation for special method
__post_init__(ANN204)
94-94: Missing type annotation for
selfin method(ANN101)
119-119: Missing return type annotation for private function
_check_statusAdd return type annotation:
None(ANN202)
119-119: Missing type annotation for
selfin method(ANN101)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
123-123: Unnecessary
elseafterraisestatement(RET506)
124-124: Replace aliased errors with
OSErrorReplace
IOErrorwith builtinOSError(UP024)
124-124: Avoid specifying long messages outside the exception class
(TRY003)
127-127: Missing type annotation for
clsin classmethod(ANN102)
145-145: Missing type annotation for
selfin method(ANN101)
145-145: Unused method argument:
query(ARG002)
145-145: Unused method argument:
operation(ARG002)
145-145: Dynamically typed expressions (typing.Any) are disallowed in
rate_limiter_key(ANN401)
154-154: Missing type annotation for
selfin method(ANN101)
159-159: Missing type annotation for
selfin method(ANN101)
168-168: Unnecessary
elseafterreturnstatement(RET505)
180-180: Missing type annotation for
selfin method(ANN101)
187-190: Use ternary operator
user_pass = f"{user}:{password}@" if password != "" else f"{user}@"instead ofif-else-blockReplace
if-else-block withuser_pass = f"{user}:{password}@" if password != "" else f"{user}@"(SIM108)
193-193: Replace aliased errors with
OSErrorReplace
IOErrorwith builtinOSError(UP024)
193-195: Avoid specifying long messages outside the exception class
(TRY003)
194-194: Trailing comma missing
Add trailing comma
(COM812)
208-208: Replace aliased errors with
OSErrorReplace
IOErrorwith builtinOSError(UP024)
208-211: Avoid specifying long messages outside the exception class
(TRY003)
210-210: Trailing comma missing
Add trailing comma
(COM812)
212-212: Unnecessary
elseafterraisestatement(RET506)
213-213: Replace aliased errors with
OSErrorReplace
IOErrorwith builtinOSError(UP024)
213-215: Avoid specifying long messages outside the exception class
(TRY003)
214-214: Trailing comma missing
Add trailing comma
(COM812)
228-228: Missing type annotation for
clsin classmethod(ANN102)
240-240: Unnecessary
elseafterreturnstatement(RET505)
254-254: Missing return type annotation for special method
__post_init__(ANN204)
254-254: Missing type annotation for
selfin method(ANN101)
281-281: Missing type annotation for
selfin method(ANN101)
287-287: Missing return type annotation for public function
cleanupAdd return type annotation:
None(ANN201)
287-287: Missing type annotation for
selfin method(ANN101)
291-291: Unnecessary
passstatementRemove unnecessary
pass(PIE790)
294-294: Missing type annotation for
selfin method(ANN101)
297-297: Missing type annotation for
selfin method(ANN101)
297-297: Missing type annotation for function argument
query(ANN001)
314-314: Missing type annotation for
selfin method(ANN101)
324-324: Missing type annotation for
selfin method(ANN101)
334-334: Missing return type annotation for public function
retrieve_objectAdd return type annotation:
None(ANN201)
334-334: Missing type annotation for
selfin method(ANN101)
359-359: Missing return type annotation for private function
_makedirsAdd return type annotation:
None(ANN202)
359-359: Missing type annotation for
selfin method(ANN101)
361-361: Unnecessary parentheses on raised exception
Remove unnecessary parentheses
(RSE102)
370-370: Missing return type annotation for public function
store_objectAdd return type annotation:
None(ANN201)
370-370: Missing type annotation for
selfin method(ANN101)
380-380: Trailing comma missing
Add trailing comma
(COM812)
388-388: Missing return type annotation for public function
removeAdd return type annotation:
None(ANN201)
388-388: Missing type annotation for
selfin method(ANN101)
Additional comments not posted (3)
snakemake_storage_plugin_xrootd/__init__.py (3)
2-10: Imports look good!The import section is well-organized and includes necessary modules for the functionality.
83-87: The addition ofurl_decoratorlooks good!The
url_decoratorfield provides flexibility for URL decoration, enhancing URL management capabilities.
131-141: Example queries look good!The method provides clear and useful example queries for the storage provider.
There was a problem hiding this comment.
Actionable comments posted: 12
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- docs/intro.md (1 hunks)
- snakemake_storage_plugin_xrootd/init.py (5 hunks)
Additional context used
Path-based instructions (1)
snakemake_storage_plugin_xrootd/__init__.py (1)
Pattern
**/*.py: Do not suggest to add trailing commas.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
LanguageTool
docs/intro.md
[uncategorized] ~5-~5: Possible missing comma found.
Context: ... specifying any options relating to the URLs in which case all of the information mu...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~5-~5: Consider removing “of” to be more concise
Context: ...ions relating to the URLs in which case all of the information must be contained in the UR...(ALL_OF_THE)
Markdownlint
docs/intro.md
1-1: null
First line in a file should be a top-level heading(MD041, first-line-heading, first-line-h1)
Ruff
snakemake_storage_plugin_xrootd/__init__.py
43-43: Trailing comma missing
Add trailing comma
(COM812)
92-92: Missing return type annotation for special method
__post_init__(ANN204)
92-92: Missing type annotation for
selfin method(ANN101)
117-117: Missing return type annotation for private function
_check_statusAdd return type annotation:
None(ANN202)
117-117: Missing type annotation for
selfin method(ANN101)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Unnecessary
elseafterraisestatement(RET506)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Missing type annotation for
clsin classmethod(ANN102)
143-143: Missing type annotation for
selfin method(ANN101)
143-143: Unused method argument:
query(ARG002)
143-143: Unused method argument:
operation(ARG002)
143-143: Dynamically typed expressions (typing.Any) are disallowed in
rate_limiter_key(ANN401)
152-152: Missing type annotation for
selfin method(ANN101)
157-157: Missing type annotation for
selfin method(ANN101)
166-166: Unnecessary
elseafterreturnstatement(RET505)
178-178: Missing type annotation for
selfin method(ANN101)
185-188: Use ternary operator
user_pass = f"{user}:{password}@" if password != "" else f"{user}@"instead ofif-else-blockReplace
if-else-block withuser_pass = f"{user}:{password}@" if password != "" else f"{user}@"(SIM108)
191-193: Avoid specifying long messages outside the exception class
(TRY003)
192-192: Trailing comma missing
Add trailing comma
(COM812)
206-209: Avoid specifying long messages outside the exception class
(TRY003)
208-208: Trailing comma missing
Add trailing comma
(COM812)
210-210: Unnecessary
elseafterraisestatement(RET506)
211-213: Avoid specifying long messages outside the exception class
(TRY003)
212-212: Trailing comma missing
Add trailing comma
(COM812)
226-226: Missing type annotation for
clsin classmethod(ANN102)
238-238: Unnecessary
elseafterreturnstatement(RET505)
252-252: Missing return type annotation for special method
__post_init__(ANN204)
252-252: Missing type annotation for
selfin method(ANN101)
279-279: Missing type annotation for
selfin method(ANN101)
285-285: Missing return type annotation for public function
cleanupAdd return type annotation:
None(ANN201)
285-285: Missing type annotation for
selfin method(ANN101)
289-289: Unnecessary
passstatementRemove unnecessary
pass(PIE790)
292-292: Missing type annotation for
selfin method(ANN101)
295-295: Missing type annotation for
selfin method(ANN101)
295-295: Missing type annotation for function argument
query(ANN001)
312-312: Missing type annotation for
selfin method(ANN101)
322-322: Missing type annotation for
selfin method(ANN101)
332-332: Missing return type annotation for public function
retrieve_objectAdd return type annotation:
None(ANN201)
332-332: Missing type annotation for
selfin method(ANN101)
357-357: Missing return type annotation for private function
_makedirsAdd return type annotation:
None(ANN202)
357-357: Missing type annotation for
selfin method(ANN101)
367-367: Missing return type annotation for public function
store_objectAdd return type annotation:
None(ANN201)
367-367: Missing type annotation for
selfin method(ANN101)
377-377: Trailing comma missing
Add trailing comma
(COM812)
385-385: Missing return type annotation for public function
removeAdd return type annotation:
None(ANN201)
385-385: Missing type annotation for
selfin method(ANN101)
Additional comments not posted (4)
snakemake_storage_plugin_xrootd/__init__.py (4)
29-33: LGTM!The
XRootDFatalExceptionclass is well-defined and serves its purpose of preventing retries for certain errors.
Line range hint
44-87: LGTM!The addition of the
url_decoratorfield inStorageProviderSettingsenhances flexibility in URL handling.Tools
Ruff
92-92: Missing return type annotation for special method
__post_init__(ANN204)
92-92: Missing type annotation for
selfin method(ANN101)
117-117: Missing return type annotation for private function
_check_statusAdd return type annotation:
None(ANN202)
117-117: Missing type annotation for
selfin method(ANN101)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Unnecessary
elseafterraisestatement(RET506)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Missing type annotation for
clsin classmethod(ANN102)
143-143: Missing type annotation for
selfin method(ANN101)
143-143: Unused method argument:
query(ARG002)
143-143: Unused method argument:
operation(ARG002)
143-143: Dynamically typed expressions (typing.Any) are disallowed in
rate_limiter_key(ANN401)
152-152: Missing type annotation for
selfin method(ANN101)
157-157: Missing type annotation for
selfin method(ANN101)
166-166: Unnecessary
elseafterreturnstatement(RET505)
178-178: Missing type annotation for
selfin method(ANN101)
185-188: Use ternary operator
user_pass = f"{user}:{password}@" if password != "" else f"{user}@"instead ofif-else-blockReplace
if-else-block withuser_pass = f"{user}:{password}@" if password != "" else f"{user}@"(SIM108)
191-193: Avoid specifying long messages outside the exception class
(TRY003)
192-192: Trailing comma missing
Add trailing comma
(COM812)
206-209: Avoid specifying long messages outside the exception class
(TRY003)
208-208: Trailing comma missing
Add trailing comma
(COM812)
210-210: Unnecessary
elseafterraisestatement(RET506)
211-213: Avoid specifying long messages outside the exception class
(TRY003)
212-212: Trailing comma missing
Add trailing comma
(COM812)
226-226: Missing type annotation for
clsin classmethod(ANN102)
238-238: Unnecessary
elseafterreturnstatement(RET505)
252-252: Missing return type annotation for special method
__post_init__(ANN204)
252-252: Missing type annotation for
selfin method(ANN101)
261-261: Missing return type annotation for public function
inventoryAdd return type annotation:
None(ANN201)
261-261: Missing type annotation for
selfin method(ANN101)
35-38: Add a return type annotation for_raise_fatal_error.Consider adding a return type annotation to indicate that this function returns
None.def _raise_fatal_error(exception: Type[Exception]) -> None:Likely invalid or redundant comment.
247-259: Ensure__post_init__is called afteris_valid_query.The comment suggests uncertainty about whether
is_valid_queryis called before__post_init__. Ensure this sequence is correct to avoid potential issues.Tools
Ruff
252-252: Missing return type annotation for special method
__post_init__(ANN204)
252-252: Missing type annotation for
selfin method(ANN101)
|
@johanneskoester in the end I put back the options for specifying the host/port/username/password as optional arguments so that it can be used with "fully-formed" URLs or simpler ones which specify some of the information via the storage provider as they could still be useful (and not that much extra effort to include them). I did have one question however but I was unsure if this is just because I missed something or if it is the expected behaviour: storage:
provider="xrootd",
retrieve=False,
host="eosuser.cern.ch"
rule some_rule:
input: storage.xrootd("root://eos/user/m/mmonk/some_file.root")
output: "test.root"
shell: "python some_script.py {input} {output}"I can see internally it correctly renders the URL to " Sorry if that's a bit of an unclear question, hopefully that makes sense! |
|
Good point. I did not foresee that there might be the requirement to return a modified URL when retrieve is set to false. I now think about how to best implement that. |
|
Btw. do you want to become a maintainer of this plugin? You are certainly much better suited than I am. |
|
@MattMonk I have added a skeleton for modifying the query. This uses the new method postprocess_query from |
|
Aha fantastic! Many thanks for putting the Yeah I'd be more than happy to be added as a maintainer for this, thanks! |
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range comments (1)
snakemake_storage_plugin_xrootd/__init__.py (1)
Line range hint
255-295: Remove unnecessarypassstatement.The
passstatement is not needed in this context as the method is already defined with a docstring.- passTools
Ruff
280-280: Missing type annotation for
selfin method(ANN101)
285-285: Missing type annotation for
selfin method(ANN101)
291-291: Missing return type annotation for public function
cleanupAdd return type annotation:
None(ANN201)
291-291: Missing type annotation for
selfin method(ANN101)
295-295: Unnecessary
passstatementRemove unnecessary
pass(PIE790)
298-298: Missing type annotation for
selfin method(ANN101)
301-301: Missing type annotation for
selfin method(ANN101)
301-301: Missing type annotation for function argument
query(ANN001)
318-318: Missing type annotation for
selfin method(ANN101)
328-328: Missing type annotation for
selfin method(ANN101)
338-338: Missing return type annotation for public function
retrieve_objectAdd return type annotation:
None(ANN201)
338-338: Missing type annotation for
selfin method(ANN101)
363-363: Missing return type annotation for private function
_makedirsAdd return type annotation:
None(ANN202)
363-363: Missing type annotation for
selfin method(ANN101)
373-373: Missing return type annotation for public function
store_objectAdd return type annotation:
None(ANN201)
373-373: Missing type annotation for
selfin method(ANN101)
383-383: Trailing comma missing
Add trailing comma
(COM812)
391-391: Missing return type annotation for public function
removeAdd return type annotation:
None(ANN201)
391-391: Missing type annotation for
selfin method(ANN101)
407-407: Missing type annotation for
selfin method(ANN101)
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
Files selected for processing (2)
- docs/further.md (1 hunks)
- snakemake_storage_plugin_xrootd/init.py (5 hunks)
Additional context used
Path-based instructions (1)
snakemake_storage_plugin_xrootd/__init__.py (1)
Pattern
**/*.py: Do not suggest to add trailing commas for improving formatting.
Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not explicitly return anything.
Markdownlint
docs/further.md
1-1: null
First line in a file should be a top-level heading(MD041, first-line-heading, first-line-h1)
Ruff
snakemake_storage_plugin_xrootd/__init__.py
43-43: Trailing comma missing
Add trailing comma
(COM812)
92-92: Missing return type annotation for special method
__post_init__(ANN204)
92-92: Missing type annotation for
selfin method(ANN101)
117-117: Missing return type annotation for private function
_check_statusAdd return type annotation:
None(ANN202)
117-117: Missing type annotation for
selfin method(ANN101)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Unnecessary
elseafterraisestatement(RET506)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Missing type annotation for
clsin classmethod(ANN102)
143-143: Missing type annotation for
selfin method(ANN101)
143-143: Unused method argument:
query(ARG002)
143-143: Unused method argument:
operation(ARG002)
143-143: Dynamically typed expressions (typing.Any) are disallowed in
rate_limiter_key(ANN401)
152-152: Missing type annotation for
selfin method(ANN101)
157-157: Missing type annotation for
selfin method(ANN101)
166-166: Unnecessary
elseafterreturnstatement(RET505)
174-174: Unnecessary
elseafterreturnstatement(RET505)
181-181: Missing type annotation for
selfin method(ANN101)
188-191: Use ternary operator
user_pass = f"{user}:{password}@" if password != "" else f"{user}@"instead ofif-else-blockReplace
if-else-block withuser_pass = f"{user}:{password}@" if password != "" else f"{user}@"(SIM108)
194-196: Avoid specifying long messages outside the exception class
(TRY003)
195-195: Trailing comma missing
Add trailing comma
(COM812)
209-212: Avoid specifying long messages outside the exception class
(TRY003)
211-211: Trailing comma missing
Add trailing comma
(COM812)
213-213: Unnecessary
elseafterraisestatement(RET506)
214-216: Avoid specifying long messages outside the exception class
(TRY003)
215-215: Trailing comma missing
Add trailing comma
(COM812)
229-229: Missing type annotation for
clsin classmethod(ANN102)
241-241: Unnecessary
elseafterreturnstatement(RET505)
244-244: Missing type annotation for
selfin method(ANN101)
260-260: Missing return type annotation for special method
__post_init__(ANN204)
260-260: Missing type annotation for
selfin method(ANN101)
285-285: Missing type annotation for
selfin method(ANN101)
291-291: Missing return type annotation for public function
cleanupAdd return type annotation:
None(ANN201)
291-291: Missing type annotation for
selfin method(ANN101)
295-295: Unnecessary
passstatementRemove unnecessary
pass(PIE790)
298-298: Missing type annotation for
selfin method(ANN101)
301-301: Missing type annotation for
selfin method(ANN101)
301-301: Missing type annotation for function argument
query(ANN001)
318-318: Missing type annotation for
selfin method(ANN101)
328-328: Missing type annotation for
selfin method(ANN101)
338-338: Missing return type annotation for public function
retrieve_objectAdd return type annotation:
None(ANN201)
338-338: Missing type annotation for
selfin method(ANN101)
363-363: Missing return type annotation for private function
_makedirsAdd return type annotation:
None(ANN202)
363-363: Missing type annotation for
selfin method(ANN101)
373-373: Missing return type annotation for public function
store_objectAdd return type annotation:
None(ANN201)
373-373: Missing type annotation for
selfin method(ANN101)
383-383: Trailing comma missing
Add trailing comma
(COM812)
391-391: Missing return type annotation for public function
removeAdd return type annotation:
None(ANN201)
391-391: Missing type annotation for
selfin method(ANN101)
Additional comments not posted (6)
snakemake_storage_plugin_xrootd/__init__.py (6)
2-3: LGTM!The imports and the custom exception class
XRootDFatalExceptionare appropriate for the functionality being implemented.Also applies to: 8-10, 29-33
Line range hint
44-85: LGTM!The
StorageProviderSettingsclass is well-defined, and the addition of theurl_decoratorfield enhances URL customization flexibility.Tools
Ruff
92-92: Missing return type annotation for special method
__post_init__(ANN204)
92-92: Missing type annotation for
selfin method(ANN101)
117-117: Missing return type annotation for private function
_check_statusAdd return type annotation:
None(ANN202)
117-117: Missing type annotation for
selfin method(ANN101)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Unnecessary
elseafterraisestatement(RET506)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Missing type annotation for
clsin classmethod(ANN102)
143-143: Missing type annotation for
selfin method(ANN101)
143-143: Unused method argument:
query(ARG002)
143-143: Unused method argument:
operation(ARG002)
143-143: Dynamically typed expressions (typing.Any) are disallowed in
rate_limiter_key(ANN401)
152-152: Missing type annotation for
selfin method(ANN101)
157-157: Missing type annotation for
selfin method(ANN101)
166-166: Unnecessary
elseafterreturnstatement(RET505)
174-174: Unnecessary
elseafterreturnstatement(RET505)
181-181: Missing type annotation for
selfin method(ANN101)
188-191: Use ternary operator
user_pass = f"{user}:{password}@" if password != "" else f"{user}@"instead ofif-else-blockReplace
if-else-block withuser_pass = f"{user}:{password}@" if password != "" else f"{user}@"(SIM108)
194-196: Avoid specifying long messages outside the exception class
(TRY003)
195-195: Trailing comma missing
Add trailing comma
(COM812)
209-212: Avoid specifying long messages outside the exception class
(TRY003)
211-211: Trailing comma missing
Add trailing comma
(COM812)
213-213: Unnecessary
elseafterraisestatement(RET506)
214-216: Avoid specifying long messages outside the exception class
(TRY003)
215-215: Trailing comma missing
Add trailing comma
(COM812)
229-229: Missing type annotation for
clsin classmethod(ANN102)
241-241: Unnecessary
elseafterreturnstatement(RET505)
244-244: Missing type annotation for
selfin method(ANN101)
260-260: Missing return type annotation for special method
__post_init__(ANN204)
260-260: Missing type annotation for
selfin method(ANN101)
267-267: Missing return type annotation for public function
inventoryAdd return type annotation:
None(ANN201)
267-267: Missing type annotation for
selfin method(ANN101)
125-139: LGTM!The
example_queriesmethod provides clear and useful examples for users.Tools
Ruff
125-125: Missing type annotation for
clsin classmethod(ANN102)
142-157: LGTM!The rate limiter methods provide basic functionality suitable for the current implementation.
Tools
Ruff
143-143: Missing type annotation for
selfin method(ANN101)
143-143: Unused method argument:
query(ARG002)
143-143: Unused method argument:
operation(ARG002)
143-143: Dynamically typed expressions (typing.Any) are disallowed in
rate_limiter_key(ANN401)
152-152: Missing type annotation for
selfin method(ANN101)
157-157: Missing type annotation for
selfin method(ANN101)
297-401: LGTM!The file operation methods are well-implemented with appropriate retry logic and error handling.
Tools
Ruff
298-298: Missing type annotation for
selfin method(ANN101)
301-301: Missing type annotation for
selfin method(ANN101)
301-301: Missing type annotation for function argument
query(ANN001)
318-318: Missing type annotation for
selfin method(ANN101)
328-328: Missing type annotation for
selfin method(ANN101)
338-338: Missing return type annotation for public function
retrieve_objectAdd return type annotation:
None(ANN201)
338-338: Missing type annotation for
selfin method(ANN101)
363-363: Missing return type annotation for private function
_makedirsAdd return type annotation:
None(ANN202)
363-363: Missing type annotation for
selfin method(ANN101)
373-373: Missing return type annotation for public function
store_objectAdd return type annotation:
None(ANN201)
373-373: Missing type annotation for
selfin method(ANN101)
383-383: Trailing comma missing
Add trailing comma
(COM812)
391-391: Missing return type annotation for public function
removeAdd return type annotation:
None(ANN201)
391-391: Missing type annotation for
selfin method(ANN101)
Line range hint
405-411: LGTM!The
list_candidate_matchesmethod is appropriately marked with a TODO comment and raises aNotImplementedErrorfor future implementation.Tools
Ruff
280-280: Missing type annotation for
selfin method(ANN101)
285-285: Missing type annotation for
selfin method(ANN101)
291-291: Missing return type annotation for public function
cleanupAdd return type annotation:
None(ANN201)
291-291: Missing type annotation for
selfin method(ANN101)
295-295: Unnecessary
passstatementRemove unnecessary
pass(PIE790)
298-298: Missing type annotation for
selfin method(ANN101)
301-301: Missing type annotation for
selfin method(ANN101)
301-301: Missing type annotation for function argument
query(ANN001)
318-318: Missing type annotation for
selfin method(ANN101)
328-328: Missing type annotation for
selfin method(ANN101)
338-338: Missing return type annotation for public function
retrieve_objectAdd return type annotation:
None(ANN201)
338-338: Missing type annotation for
selfin method(ANN101)
363-363: Missing return type annotation for private function
_makedirsAdd return type annotation:
None(ANN202)
363-363: Missing type annotation for
selfin method(ANN101)
373-373: Missing return type annotation for public function
store_objectAdd return type annotation:
None(ANN201)
373-373: Missing type annotation for
selfin method(ANN101)
383-383: Trailing comma missing
Add trailing comma
(COM812)
391-391: Missing return type annotation for public function
removeAdd return type annotation:
None(ANN201)
391-391: Missing type annotation for
selfin method(ANN101)
407-407: Missing type annotation for
selfin method(ANN101)
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- snakemake_storage_plugin_xrootd/init.py (5 hunks)
Additional context used
Path-based instructions (1)
snakemake_storage_plugin_xrootd/__init__.py (1)
Pattern
**/*.py: Do not suggest to add trailing commas for improving formatting.
Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not explicitly return anything.
Ruff
snakemake_storage_plugin_xrootd/__init__.py
43-43: Trailing comma missing
Add trailing comma
(COM812)
92-92: Missing return type annotation for special method
__post_init__(ANN204)
92-92: Missing type annotation for
selfin method(ANN101)
117-117: Missing return type annotation for private function
_check_statusAdd return type annotation:
None(ANN202)
117-117: Missing type annotation for
selfin method(ANN101)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
124-124: Missing type annotation for
clsin classmethod(ANN102)
142-142: Missing type annotation for
selfin method(ANN101)
142-142: Unused method argument:
query(ARG002)
142-142: Unused method argument:
operation(ARG002)
142-142: Dynamically typed expressions (typing.Any) are disallowed in
rate_limiter_key(ANN401)
151-151: Missing type annotation for
selfin method(ANN101)
156-156: Missing type annotation for
selfin method(ANN101)
178-178: Missing type annotation for
selfin method(ANN101)
185-188: Use ternary operator
user_pass = f"{user}:{password}@" if password != "" else f"{user}@"instead ofif-else-blockReplace
if-else-block withuser_pass = f"{user}:{password}@" if password != "" else f"{user}@"(SIM108)
191-193: Avoid specifying long messages outside the exception class
(TRY003)
192-192: Trailing comma missing
Add trailing comma
(COM812)
206-209: Avoid specifying long messages outside the exception class
(TRY003)
208-208: Trailing comma missing
Add trailing comma
(COM812)
210-210: Unnecessary
elseafterraisestatement(RET506)
211-213: Avoid specifying long messages outside the exception class
(TRY003)
212-212: Trailing comma missing
Add trailing comma
(COM812)
226-226: Missing type annotation for
clsin classmethod(ANN102)
240-240: Missing type annotation for
selfin method(ANN101)
256-256: Missing return type annotation for special method
__post_init__(ANN204)
256-256: Missing type annotation for
selfin method(ANN101)
281-281: Missing type annotation for
selfin method(ANN101)
287-287: Missing return type annotation for public function
cleanupAdd return type annotation:
None(ANN201)
287-287: Missing type annotation for
selfin method(ANN101)
291-291: Unnecessary
passstatementRemove unnecessary
pass(PIE790)
294-294: Missing type annotation for
selfin method(ANN101)
297-297: Missing type annotation for
selfin method(ANN101)
297-297: Missing type annotation for function argument
query(ANN001)
314-314: Missing type annotation for
selfin method(ANN101)
324-324: Missing type annotation for
selfin method(ANN101)
334-334: Missing return type annotation for public function
retrieve_objectAdd return type annotation:
None(ANN201)
334-334: Missing type annotation for
selfin method(ANN101)
359-359: Missing return type annotation for private function
_makedirsAdd return type annotation:
None(ANN202)
359-359: Missing type annotation for
selfin method(ANN101)
369-369: Missing return type annotation for public function
store_objectAdd return type annotation:
None(ANN201)
369-369: Missing type annotation for
selfin method(ANN101)
379-379: Trailing comma missing
Add trailing comma
(COM812)
387-387: Missing return type annotation for public function
removeAdd return type annotation:
None(ANN201)
387-387: Missing type annotation for
selfin method(ANN101)
Additional comments not posted (13)
snakemake_storage_plugin_xrootd/__init__.py (13)
2-4: Imports look good.The imports are appropriate and consistent with the functionality provided in the file.
29-33: Custom exception class is well-defined.The
XRootDFatalExceptionclass is appropriately defined for handling non-retryable XRootD errors.
35-38: Error handling function is correctly implemented.The
_raise_fatal_errorfunction is well-suited for handling fatal exceptions by logging and re-raising them.
41-44: Retry settings are hardcoded as per current requirements.While making them configurable could add flexibility, the current setup is acceptable based on previous discussions.
Tools
Ruff
43-43: Trailing comma missing
Add trailing comma
(COM812)
81-85: Addition ofurl_decoratorenhances flexibility.The
url_decoratorfield inStorageProviderSettingsallows for more flexible URL construction, which is a beneficial enhancement.
240-243: Postprocess query method is correctly implemented.The
postprocess_querymethod effectively utilizes the_parse_urlmethod to modify the query URL.Tools
Ruff
240-240: Missing type annotation for
selfin method(ANN101)
251-259: Initialization logic inStorageObjectis appropriate.The
__post_init__method correctly initializes attributes using the_parse_urlmethod.Tools
Ruff
256-256: Missing return type annotation for special method
__post_init__(ANN204)
256-256: Missing type annotation for
selfin method(ANN101)
293-309: Existence check methods are well-implemented.The
existsand_existsmethods correctly handle file existence checks using XRootD status.Tools
Ruff
294-294: Missing type annotation for
selfin method(ANN101)
297-297: Missing type annotation for
selfin method(ANN101)
297-297: Missing type annotation for function argument
query(ANN001)
313-330: Modification time and size retrieval methods are correct.The
mtimeandsizemethods are well-implemented and handle status checks appropriately.Tools
Ruff
314-314: Missing type annotation for
selfin method(ANN101)
324-324: Missing type annotation for
selfin method(ANN101)
333-353: Retrieve object method is correctly implemented.The
retrieve_objectmethod effectively usesclient.CopyProcessto download files and handles status checks.Tools
Ruff
334-334: Missing return type annotation for public function
retrieve_objectAdd return type annotation:
None(ANN201)
334-334: Missing type annotation for
selfin method(ANN101)
368-384: Store object method is correctly implemented.The
store_objectmethod effectively usesclient.CopyProcessto upload files and handles status checks.Tools
Ruff
369-369: Missing return type annotation for public function
store_objectAdd return type annotation:
None(ANN201)
369-369: Missing type annotation for
selfin method(ANN101)
379-379: Trailing comma missing
Add trailing comma
(COM812)
386-396: Remove method is correctly implemented.The
removemethod effectively deletes files or directories and handles status checks.Tools
Ruff
387-387: Missing return type annotation for public function
removeAdd return type annotation:
None(ANN201)
387-387: Missing type annotation for
selfin method(ANN101)
Line range hint
401-409: Placeholder for listing candidate matches is appropriate.The
list_candidate_matchesmethod is correctly marked as not implemented, with a TODO comment indicating future support.Tools
Ruff
276-276: Missing type annotation for
selfin method(ANN101)
281-281: Missing type annotation for
selfin method(ANN101)
287-287: Missing return type annotation for public function
cleanupAdd return type annotation:
None(ANN201)
287-287: Missing type annotation for
selfin method(ANN101)
291-291: Unnecessary
passstatementRemove unnecessary
pass(PIE790)
294-294: Missing type annotation for
selfin method(ANN101)
297-297: Missing type annotation for
selfin method(ANN101)
297-297: Missing type annotation for function argument
query(ANN001)
314-314: Missing type annotation for
selfin method(ANN101)
324-324: Missing type annotation for
selfin method(ANN101)
334-334: Missing return type annotation for public function
retrieve_objectAdd return type annotation:
None(ANN201)
334-334: Missing type annotation for
selfin method(ANN101)
359-359: Missing return type annotation for private function
_makedirsAdd return type annotation:
None(ANN202)
359-359: Missing type annotation for
selfin method(ANN101)
369-369: Missing return type annotation for public function
store_objectAdd return type annotation:
None(ANN201)
369-369: Missing type annotation for
selfin method(ANN101)
379-379: Trailing comma missing
Add trailing comma
(COM812)
387-387: Missing return type annotation for public function
removeAdd return type annotation:
None(ANN201)
387-387: Missing type annotation for
selfin method(ANN101)
403-403: Missing type annotation for
selfin method(ANN101)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- snakemake_storage_plugin_xrootd/init.py (5 hunks)
Additional context used
Path-based instructions (1)
snakemake_storage_plugin_xrootd/__init__.py (1)
Pattern
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Learnings (1)
snakemake_storage_plugin_xrootd/__init__.py (2)
Learnt from: johanneskoester PR: snakemake/snakemake-storage-plugin-xrootd#8 File: snakemake_storage_plugin_xrootd/__init__.py:358-367 Timestamp: 2024-08-20T11:08:05.501Z Learning: Do not suggest return type annotations for functions or methods that do not contain a `return` statement.Learnt from: johanneskoester PR: snakemake/snakemake-storage-plugin-xrootd#8 File: snakemake_storage_plugin_xrootd/__init__.py:117-121 Timestamp: 2024-08-20T11:07:48.841Z Learning: Do not suggest return type annotations for functions or methods that do not contain a `return` statement.
Additional comments not posted (21)
snakemake_storage_plugin_xrootd/__init__.py (21)
31-34: Custom exception class for XRootD errors looks good.The
XRootDFatalExceptionclass is well-defined and serves its purpose in preventing retries for certain XRootD errors.
37-40: Error handling function is correctly implemented.The
_raise_fatal_errorfunction properly raisesXRootDFatalExceptionand logs a warning.
43-46: Hardcoded retry settings are acceptable for now.The retry settings are hardcoded, and a TODO comment suggests making them configurable in the future. This is acceptable given the current requirements.
83-87: Enhanced URL management withurl_decorator.The addition of the
url_decoratorfield inStorageProviderSettingsenhances flexibility in URL management, while the removal ofquery_paramssimplifies configuration.
119-123: Error handling in_check_statusis well-implemented.The
_check_statusmethod correctly raisesXRootDFatalExceptionfor non-retryable errors andWorkflowErrorfor others.
126-140: Example queries are clear and informative.The
example_queriesmethod provides useful examples of valid queries for the XRootD storage provider.
143-151: Rate limiter key is correctly implemented.The
rate_limiter_keymethod returns a constant key for rate limiting, which is appropriate for XRootD.
162-167: Password obfuscation in URLs is well-handled.The
_no_pass_urlmethod effectively removes passwords from URLs for safe printing.
169-174: Parameter obfuscation in URLs is well-handled.The
_no_params_urlmethod effectively removes parameters from URLs for safe printing.
176-178: Comprehensive URL obfuscation is well-implemented.The
_safe_to_print_urlmethod combines password and parameter obfuscation for safe URL printing.
180-228: URL parsing and construction are well-implemented.The
_parse_urlmethod effectively handles user and password combinations using Python'smatchstatement, ensuring valid URL construction.
245-248: Query postprocessing with URL decorator is effective.The
postprocess_querymethod correctly applies the URL decorator to enhance query functionality.
262-265: Initialization in__post_init__is well-implemented.The
__post_init__method correctly initializes URL parsing and file system setup forStorageObject.
298-300: File existence check with retry mechanism is effective.The
existsmethod correctly checks for file existence using the_existshelper and the retry mechanism.
302-315: Detailed file existence check is well-implemented.The
_existsmethod correctly handles file existence checks and specific error codes, ensuring robust error handling.
318-325: Modification time retrieval is well-implemented.The
mtimemethod correctly retrieves the modification time of a file and handles errors appropriately.
328-335: File size retrieval is well-implemented.The
sizemethod correctly retrieves the size of a file and handles errors appropriately.
338-358: File retrieval is well-implemented.The
retrieve_objectmethod correctly handles file downloads to a local path and includes robust error checking.
364-371: Directory creation is well-implemented.The
_makedirsmethod correctly ensures necessary directories are created before file operations.
373-389: File storage is well-implemented.The
store_objectmethod correctly handles file uploads to a remote location and includes robust error checking.
391-401: File removal is well-implemented.The
removemethod correctly handles file deletions from storage and includes robust error checking.
|
One more thing I've realised is that if a password is supplied this will of course end up in the root url like I guess then if we want to keep this option some change would have to be made so that when snakemake prints out the input/output files when it runs the rule to use the censored version instead. I'm not sure how much work that would be, possibly for now we could drop support for using a password with this plugin and add it back later or possibly not at all if it's a bit too much of a risk having something that puts your password in plaintext in the URL it uses to access the file? I think using the password field in the URL is not the usual way of authenticating remote files with it anyway |
|
Yes, good point. On the other hand, this will mostly be used in trusted environments. My suggestion is to keep that functionality, but issue a warning with logger.warning() and also put a warning into the help text of the password setting. |
|
Okay yeah that makes sense! I'll do that and then I think otherwise this is ready to go from my side |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- docs/intro.md (1 hunks)
- snakemake_storage_plugin_xrootd/init.py (5 hunks)
Files skipped from review due to trivial changes (1)
- docs/intro.md
Additional context used
Path-based instructions (1)
snakemake_storage_plugin_xrootd/__init__.py (1)
Pattern
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Learnings (1)
snakemake_storage_plugin_xrootd/__init__.py (2)
Learnt from: johanneskoester PR: snakemake/snakemake-storage-plugin-xrootd#8 File: snakemake_storage_plugin_xrootd/__init__.py:358-367 Timestamp: 2024-08-20T11:08:05.501Z Learning: Do not suggest return type annotations for functions or methods that do not contain a `return` statement.Learnt from: johanneskoester PR: snakemake/snakemake-storage-plugin-xrootd#8 File: snakemake_storage_plugin_xrootd/__init__.py:117-121 Timestamp: 2024-08-20T11:07:48.841Z Learning: Do not suggest return type annotations for functions or methods that do not contain a `return` statement.
Additional comments not posted (14)
snakemake_storage_plugin_xrootd/__init__.py (14)
2-3: Imports are appropriate for type hinting.The imported modules from
typingare suitable for the type annotations used in the code.
31-34: Custom exceptionXRootDFatalExceptionis well-defined.This class is a clear and specific way to handle unrecoverable errors in the XRootD context.
37-40: Function_raise_fatal_erroris well-implemented.The function effectively handles fatal exceptions with appropriate logging.
43-46: Retry configuration is clear and appropriate.The retry settings are well-defined, and the use of
_raise_fatal_erroras a callback is suitable for handling fatal errors. The decision not to make it configurable is acknowledged.
86-90:url_decoratorfield adds flexibility for URL manipulation.This field allows for customizable URL modifications, enhancing the provider's flexibility.
98-127: Initialization and no-retry codes are well-defined.The
__post_init__method thoroughly initializes settings, and the no-retry error codes are clearly specified.
129-133:_check_statusmethod effectively handles error checking.The method distinguishes between fatal and retryable errors, using
WorkflowErrorfor user-exposed issues.
140-150:example_queriesmethod provides clear examples.The examples and descriptions are helpful for users constructing queries for the XRootD storage provider.
172-188: URL helper methods enhance security and robustness.The methods effectively remove sensitive information from URLs, improving security.
190-238:_parse_urlmethod is well-implemented withmatchstatement.The method simplifies URL construction and validation, effectively handling user and password combinations.
255-258:postprocess_querymethod effectively applies global settings.The method ensures consistent URL handling by integrating
_parse_urllogic.
272-275:__post_init__method inStorageObjectis well-defined.The method ensures correct configuration with parsed URL and file system setup.
308-411: File operation methods inStorageObjectare robust and resilient.The methods effectively use
xrootd_retryand_check_statusfor error handling and resilience against transient errors.
373-382:_makedirsmethod is correctly implemented for directory creation.The method ensures necessary directories are created, using retry and error handling mechanisms.
🤖 I have created a release *beep* *boop* --- ## [0.1.3](v0.1.2...v0.1.3) (2024-08-20) ### Bug Fixes * Fix XRootD storage plugin at least for file access ([#8](#8)) ([b7981cc](b7981cc)) * update dependencies ([#9](#9)) ([417e73d](417e73d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR updates the storage plugin to allow read/write file access with XRootD, borrowing heavily from the XRootD remote provider in
Snakemake<8to allow remote input/output files.It should also be possible to support
StorageObjectGlobbut I would have to think about it a bit more (or of course someone else would also be more than welcome to have a go!).I tested this with the latest Snakemake (commit
e8735c14from a local editable install withPython 3.12.4) and could successfully:retrieve=Falseretrieve=TrueSummary by CodeRabbit
New Features
Bug Fixes
Documentation