Allow setting a few global defaults for file source plugin types. #18909
Conversation
jmchilton
added
kind/enhancement
area/admin
area/configuration
jmchilton
force-pushed
the
file_source_property_global_defaults
branch
13 times, most recently
from
5849b06
to
1899eb1
Compare
|
I also fixed up and expanded some webdav tests. Directions are at the top of the test_webdav.py if you want to run them. |
| @@ -75,6 +91,9 @@ def from_dict(as_dict): | |||
| user_library_import_dir=as_dict["user_library_import_dir"], | |||
| ftp_upload_dir=as_dict["ftp_upload_dir"], | |||
| ftp_upload_purge=as_dict["ftp_upload_purge"], | |||
| tmp_dir=as_dict.get("tmp_dir"), | |||
There was a problem hiding this comment.
| tmp_dir=as_dict.get("tmp_dir"), | |
| # Always provided, remove in 25.0 | |
| tmp_dir=as_dict.get("tmp_dir"), |
?
|
|
||
| file_source_webdav_use_temp_files: | ||
| type: bool | ||
| default: false |
There was a problem hiding this comment.
Why is the default false ? Could we not hardcode this to true ? When would you ever want this to be false ? I feel like we should just patch up quirks in file sources, not burden admins with this stuff.
There was a problem hiding this comment.
I agree, I missed that default to false. In any case, it should be true. And yes, I can't find a reason someone would want it to be false. So maybe we could just remove it from the config and leave it as default which will be true, the only important part that needs to be controlled by the admin is the temp directory.
There was a problem hiding this comment.
I will switch the default to True - that was an oversight, the actual underlying plugin defaults to False (https://github.com/PyFilesystem/webdavfs/blob/master/webdavfs/webdavfs.py#L167) - so it seems like it should be configurable to me but I will drop it if y'all insist.
There was a problem hiding this comment.
Ahh I see, that is because temp_path is also None by default at the PyFileSystem plugin level, but in our file source plugin we probably can fix it always to be true, since we have (now) the temp_path set in the config or the fallback in case of None.
| file_source_s3fs_listings_expiry_time: | ||
| type: int | ||
| default: 60 | ||
| desc: | | ||
| Default value for listings_expiry_time for s3fs file source plugins that don't explicitly declare this. |
There was a problem hiding this comment.
| file_source_s3fs_listings_expiry_time: | |
| type: int | |
| default: 60 | |
| desc: | | |
| Default value for listings_expiry_time for s3fs file source plugins that don't explicitly declare this. | |
| file_source_listings_expiry_time: | |
| type: int | |
| default: 60 | |
| desc: | | |
| Number of seconds before file source content listings are refreshed. Shorter times will result in more queries while browsing a file sources. Longer times will result in fewer requests to file sources but outdated contents might be displayed to the user. Currently only affects s3fs file sources. |
This is more reasonable to make configurable, but it still seems like a setting no-one should need to touch. We might have to implement for other file sources if request volume becomes problematic, hence dropping the s3fs from the setting name.
There was a problem hiding this comment.
It only affects s3fs - I feel like changing it makes it more likely people will try to fix other timeouts by adjusting it. I will make this change but I'm grumpy about it.
There was a problem hiding this comment.
You could add currently only affects s3fs file source ?
There was a problem hiding this comment.
(Updated the suggestion with that)
jmchilton
force-pushed
the
file_source_property_global_defaults
branch
from
1899eb1
to
34c1a3a
Compare
jmchilton
force-pushed
the
file_source_property_global_defaults
branch
from
34c1a3a
to
dfc194d
Compare
The idea is we don't want users having to define the technical details of plugin specification that are related to the infrastructure of Galaxy and not the remote data source. Even allowing them in the templates is kind of heavy - so being able to just set them in Galaxy's config is sort of ideal.
Even outside the context of user defined file sources - I think this is kind of an important point. Take for example - https://github.com/galaxyproject/usegalaxy-playbook/blob/main/env/common/templates/galaxy/config/file_sources_conf.yml.j2 - it is very heavy and error prone that the "listings_expiry_time" parameter needs to be defined in each plugin.
How to test the changes?
(Select all options that apply)
License