Skip to content

Commit

Permalink
Fix startup when data_manager_config_file config option is set to…
Browse files Browse the repository at this point in the history
… an empty value

by not resolving it to the `path_resolves_to` directory in such a case.

planemo (which we use in the converters tests) set this option to an
empty value when not testing data managers:

https://github.com/galaxyproject/planemo/blob/master/planemo/galaxy/config.py#L452

Resulting in this traceback:

```
Traceback (most recent call last):
  File "/usr/users/ga002/soranzon/software/nsoranzo_galaxy/lib/galaxy/webapps/galaxy/buildapp.py", line 68, in app_pair
    app = galaxy.app.UniverseApplication(global_conf=global_conf, is_webapp=True, **kwargs)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/users/ga002/soranzon/software/nsoranzo_galaxy/lib/galaxy/app.py", line 777, in __init__
    self.data_managers = self._register_singleton(DataManagers)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/users/ga002/soranzon/software/nsoranzo_galaxy/lib/galaxy/di.py", line 27, in _register_singleton
    instance = self[dep_type]
               ~~~~^^^^^^^^^^
  File "/usr/users/ga002/soranzon/software/nsoranzo_galaxy/.venv/lib/python3.12/site-packages/lagom/container.py", line 381, in __getitem__
    return self.resolve(dep)
           ^^^^^^^^^^^^^^^^^
  File "/usr/users/ga002/soranzon/software/nsoranzo_galaxy/.venv/lib/python3.12/site-packages/lagom/container.py", line 265, in resolve
    return self._reflection_build_with_err_handling(dep_type, suppress_error)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/users/ga002/soranzon/software/nsoranzo_galaxy/.venv/lib/python3.12/site-packages/lagom/container.py", line 392, in _reflection_build_with_err_handling
    return self._reflection_build(dep_type)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/users/ga002/soranzon/software/nsoranzo_galaxy/.venv/lib/python3.12/site-packages/lagom/container.py", line 408, in _reflection_build
    return dep_type(**sub_deps)  # type: ignore
           ^^^^^^^^^^^^^^^^^^^^
  File "/usr/users/ga002/soranzon/software/nsoranzo_galaxy/lib/galaxy/tools/data_manager/manager.py", line 44, in __init__
    self.load_from_xml(filename)
  File "/usr/users/ga002/soranzon/software/nsoranzo_galaxy/lib/galaxy/tools/data_manager/manager.py", line 58, in load_from_xml
    tree = util.parse_xml(xml_filename)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/users/ga002/soranzon/software/nsoranzo_galaxy/lib/galaxy/util/__init__.py", line 360, in parse_xml
    with source.open("rb") as f:
         ^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/pathlib.py", line 1013, in open
    return io.open(self, mode, buffering, encoding, errors, newline)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
IsADirectoryError: [Errno 21] Is a directory: '/tmp/tmps2qq5htq'
```

Why this has become a problem
-----------------------------

Before commit 3282aa3 `util.parse_xml()`
would raise a `XMLSyntaxError` exception if passed a directory, but
now it raises `IsADirectoryError`, which is a subclass of `OSError` with
errno 21.
As a consequence when `load_from_xml()` in lib/galaxy/tools/data_manager/manager.py
calls `util.parse_xml()` passing a directory, the resulting exception
is now re-raised in the `except OSError as e:` branch instead of
just being logged, which is good since this is a misconfiguration.

N.B.: This will also need to be fixed in Planemo by not setting the
`data_manager_config_file` option when not needed.
  • Loading branch information
nsoranzo committed Dec 14, 2024
1 parent 2dc6a2a commit 308dc48
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 9 deletions.
6 changes: 4 additions & 2 deletions lib/galaxy/config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -511,10 +511,10 @@ def resolve(key):
if not parent: # base case: nothing else needs resolving
return path
parent_path = resolve(parent) # recursively resolve parent path
if path is not None:
if path:
path = os.path.join(parent_path, path) # resolve path
else:
path = parent_path # or use parent path
log.warning("Trying to resolve path for the '%s' option but it's empty/None", key)

setattr(self, key, path) # update property
_cache[key] = path # cache it!
Expand Down Expand Up @@ -557,6 +557,8 @@ def get_path(current_path, initial_path):
return current_path

current_value = getattr(self, key) # resolved path or list of resolved paths
if not current_value:
return
if isinstance(current_value, list):
initial_paths = listify(self._raw_config[key], do_strip=True) # initial unresolved paths
updated_paths = []
Expand Down
2 changes: 2 additions & 0 deletions lib/galaxy/jobs/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,8 @@ def __init__(self, app: MinimalManagerApp):
f" release of Galaxy. Please convert to YAML at {self.app.config.job_config_file} or"
f" explicitly set `job_config_file` to {job_config_file} to remove this message"
)
if not job_config_file:
raise OSError()
if ".xml" in job_config_file:
tree = load(job_config_file)
job_config_dict = self.__parse_job_conf_xml(tree)
Expand Down
6 changes: 3 additions & 3 deletions test/unit/config/test_path_graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ def test_resolves_to_invalid_type(monkeypatch):


def test_resolves_with_empty_component(monkeypatch):
# A path can be None (root path is never None; may be asigned elsewhere)
# A path can be None (root path is never None; may be assigned elsewhere)
mock_schema = {
"path0": {
"type": "str",
Expand All @@ -155,13 +155,13 @@ def test_resolves_with_empty_component(monkeypatch):
"path2": {
"type": "str",
"default": "value2",
"path_resolves_to": "path1",
"path_resolves_to": "path0",
},
}
monkeypatch.setattr(AppSchema, "_read_schema", lambda a, b: get_schema(mock_schema))
monkeypatch.setattr(BaseAppConfiguration, "_load_schema", lambda a: AppSchema(Path("no path"), "_"))

config = BaseAppConfiguration()
assert config.path0 == "value0"
assert config.path1 == "value0"
assert config.path1 is None
assert config.path2 == "value0/value2"
8 changes: 4 additions & 4 deletions test/unit/config/test_path_resolves_to.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,21 +132,21 @@ def test_kwargs_relative_path_old_prefix_for_other_option(mock_init):

def test_kwargs_relative_path_old_prefix_empty_after_strip(mock_init):
# Expect: use value from kwargs, strip old prefix, then resolve
new_path1 = "old-config"
new_path1 = "old-config/foo"
config = BaseAppConfiguration(path1=new_path1)

assert config.path1 == "my-config/" # stripped of old prefix, then resolved
assert config.path1 == "my-config/foo" # stripped of old prefix, then resolved
assert config.path2 == "my-data/my-data-files" # stripped of old prefix, then resolved
assert config.path3 == "my-other-files" # no change


def test_kwargs_set_to_null(mock_init):
# Expected: allow overriding with null, then resolve
# Expected: allow overriding with null
# This is not a common scenario, but it does happen: one example is
# `job_config` set to `None` when testing
config = BaseAppConfiguration(path1=None)

assert config.path1 == "my-config" # resolved
assert config.path1 is None
assert config.path2 == "my-data/my-data-files" # resolved
assert config.path3 == "my-other-files" # no change

Expand Down

0 comments on commit 308dc48

Please sign in to comment.