[23.2] Expose file_name property in DatasetFilenameWrapper

[jdavcs]

Fixes #17098.

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[mvdbeek]

Looks good, but can you use this in one of the framework tool tests ?

[nsoranzo]

I don't think this is correct, but any way, wouldn't it be easier to revert this part: c30a4c0#diff-01abdfd70033a33dfc2f945458e601e0418956e036ace05bebaf1ed8c4e263e8L474-R476

[mvdbeek]

That would probably only work with outputs_to_working_directory: true ? What about returning str(self) ?

[nsoranzo]

That would probably only work with outputs_to_working_directory: true ? What about returning str(self) ?

That was my initial thought before looking at the commit history, it should be fine as well.

[jdavcs]

@nsoranzo Reverting that part does not fix it, rather it causes the test to fail in a new way (that I cannot understand yet, but certainly caused by a filename error). The property approach works, at least for my test case.

I'll add a framework test to expose this, which will also help me better understand how this is supposed to work. Can you elaborate on why you think the proposed solution may be incorrect? (I'm not disagreeing - just trying to better understand the problem).

[nsoranzo]

I think the correct fix is:

diff --git a/lib/galaxy/tools/wrappers.py b/lib/galaxy/tools/wrappers.py
index 9e59536c65..12094585da 100644
--- a/lib/galaxy/tools/wrappers.py
+++ b/lib/galaxy/tools/wrappers.py
@@ -464,6 +464,9 @@ class DatasetFilenameWrapper(ToolParameterValueWrapper):
                 )
         return self.dataset.datatype.matches_any(datatypes)

+    @property
+    def file_name(self) -> str:
+        return str(self)
+
     def __str__(self) -> str:
         if self.false_path is not None:
             return self.false_path
@@ -471,10 +474,7 @@ class DatasetFilenameWrapper(ToolParameterValueWrapper):
             return str(self.unsanitized.get_file_name())
 
     def __getattr__(self, key: Any) -> Any:
-        if self.false_path is not None and key == "get_file_name":
-            # Path to dataset was rewritten for this job.
-            return lambda *args, **kwargs: self.false_path
-        elif key in ("extra_files_path", "files_path"):
+        if key in ("extra_files_path", "files_path"):
             if not self.compute_environment:
                 # Only happens in WrappedParameters context, refactor!
                 return self.unsanitized.extra_files_path

The property approach works, at least for my test case.

You're right, but it works in a convoluted way.
DatasetFilenameWrapper doesn't have a get_file_name() method (self.dataset, i.e. the underlying dataset, has it), so in your solution calling it goes through the __getattr__() method and returns either self.false_path or getattr(self.dataset, key)() (i.e. self.dataset.get_file_name()), depending on the value of self.false_path .

[jdavcs]

The simplified call in the property works fine, but we can't drop the if statement from __getattr__ - that, unfortunately, doesn't work. Both keys, file_name and get_file_name are used, and with the latter, the call to getattr(self.dataset, key) produces the wrong value (a string of Xs instead of a callable). So, I have a simpler solution: just check for both keys (see latest commit). I think this works.

[mvdbeek]

Both keys, file_name and get_file_name are used,

Isn't the use of get_file_name a bug as well ? That should be purely on the the galaxy side, and I think it'd be good if we didn't expose additional properties and methods.

[jdavcs]

Both keys, file_name and get_file_name are used,

Isn't the use of get_file_name a bug as well ? That should be purely on the the galaxy side, and I think it'd be good if we didn't expose additional properties and methods.

Sorry, I am confused. I thought that wrapper provides access to all attributes of a dataset, including get_file_name. Which, apparently, is not the case. I think I'm missing a key piece here. The get_file_name is serialized as file_name - is that why the wrapper should only expect file_name and not get_file_name? Can you, pls, explain (or point me in the right direction) what kind of attributes are supposed to be accessed via the wrapper?

[nsoranzo]

Both keys, file_name and get_file_name are used,

Isn't the use of get_file_name a bug as well ? That should be purely on the the galaxy side, and I think it'd be good if we didn't expose additional properties and methods.

Sorry, I am confused. I thought that wrapper provides access to all attributes of a dataset, including get_file_name. Which, apparently, is not the case. I think I'm missing a key piece here. The get_file_name is serialized as file_name - is that why the wrapper should only expect file_name and not get_file_name? Can you, pls, explain (or point me in the right direction) what kind of attributes are supposed to be accessed via the wrapper?

The wrapper expose only selected properties that can be used when writing the Cheetah command section of a Galaxy tool. This is not documented anywhere in full, you can find examples of .file_name in https://docs.galaxyproject.org/en/master/dev/schema.html#tool-configfiles-configfile and in the tool mentioned in #17098 ( https://github.com/galaxyproject/tools-iuc/blob/main/tools/gprofiler/gprofiler_orth.xml#L33 ).
get_file_name is a new method and never been exposed to the tool command before, so need to make it available now.

[jdavcs]

test failure relevant.

[nsoranzo]

Should also

galaxy/test/unit/app/tools/test_wrappers.py

Line 210 in a216124

assert wrapper.get_file_name() == MOCK_DATASET_PATH

be reverted?

[mvdbeek]

Yes, thank you!