IUC want profile in the <tool> and detect_errors in the <command>

[fubar2]

Hi @hexylena! Request from the IUC to add boilerplate doodads to ToolFactory outputs, so this PR elevates command to an XMLParam, adding the command string as a CDATA text. It passes tests including the new one and more importantly, the existing one, though I'm not sure if this is the most efficient path. Whatever, it works for me FWIW - thanks!

[hexylena]

Is setting detect_errors to anything else than "aggressive" supported?

yes, and aggressive is generally the wrong choice, IMO. I would much, much rather see it set to exit_code

Aggressive is rather annoying when wrapping other people's tools; as soon as some library decides to log a non-fatal warning or error, the job fails, despite the job in fact succeeding by exit code/results. Too many tools have used "Error:" in their outputs that I avoid aggressive.

edit: you meant in the python side. Yes, would like a test for that. (And a diffrent default)

[hexylena]

@bernt-matthias where are you with #37, should we merge it? I'd like the github workflow that you've done there.

[bernt-matthias]

@bernt-matthias where are you with #37, should we merge it? I'd like the github workflow that you've done there.

Ready for review... Not sure if tests succeed yet. Can also separate the workflow in a separate PR if needed.

[fubar2]

Thanks @hexylena and @bernt-matthias!

Yes, defaults are both what @bgruening asked for in https://github.com/bgruening/galaxytools/pull/1326. AFAIK, those should properly be determined by the IUC I think, not me - I'm just doing what I was asked and will be happy to meet their requirements.

[bgruening](https://github.com/bgruening) [Sep 1, 2023](https://github.com/bgruening/galaxytools/pull/1326#discussion_r1312980881)

can we please add profile="22.05"

and

[bgruening](https://github.com/bgruening) [Sep 1, 2023](https://github.com/bgruening/galaxytools/pull/1326#discussion_r1312983046)

This very simple stdio is deprecated and should be replaced with <command detect_errors="aggressive">

They should both be explicitly set by the caller, like this example from the ToolFactory for profile:

        self.newtool = gxt.Tool(
            self.tool_name,
            self.tool_id,
            self.tool_version,
            self.args.tool_desc,
            self.args.sysexe,
            profile = self.profile

and here for detect_errors: self.newtool.command = gxtp.Command(detect_errors="aggressive")

These changes already have one new unit test and import_xml_sample has these elements - and they appear to be correctly parsed and set in the unit test. The ToolFactory tests them too if I use this updated galaxyxml and it passes its own tests. In summary: I am not sure exactly how to satisfy this beyond the already added unit test - hints appreciated. Are you suggesting another unit test @bernt-matthias? One to set and test different values. I could do that if you and @hexylena think it would help get this sorted out.

The defaults are the settings suggested. Perhaps too conservative and obviously the caller has the power to set them as they wish. Happy to change them if someone who is better placed to advise than Bjoern is willing to make a ruling?. Please let me know explicitly what I need to do to get this in so I can get on using it?

[fubar2]

@hexylena @bernt-matthias Does this additional unit test help? The constructor is being called from import_xml.py (python side) in the unit tests and seems to work. Do you want something outside the unit tests to do that too? So these new properties are already being read and stowed from sample xml and seem just dandy in that first new unit test, but another explicitly setting and testing the new values, confirm that they are mutable, so can be whatever the user wants as the ToolFactory constructor examples show.

Is anything not yet being tested that needs testing for these changes to be acceptable? Please let me know what else I need to do?

class TestChangeProfDet(TestImport):
    def test_changes(self):
        self.tool.command.node.attrib["detect_errors"] = "foobarfoo"
        de = self.tool.command.node.attrib["detect_errors"]
        self.assertEqual(de, "foobarfoo")
        self.tool.profile  = "anything you want"
        self.assertEqual(self.tool.profile,  "anything you want")

[bernt-matthias]

I guess the most undebatable option is to use Galaxy's defaults, ie to not set them by default.

Btw would this be done by setting the value to None?

But I'm fine with any default.

[fubar2]

Thanks @bernt-matthias - So all good? I no longer see real use so I have no experience with their effects, so changing them to anything else is fine with me. Default being None makes them required at init I guess which might be wise if they are critically important, like an id. Strict defaults for anyone not making their own choice seems better than making them required to me FWIW. ToolFactory generated tools will have those values because that's what @bgruening asked for.

[bernt-matthias]

For your PRs I would suggest to go with marius' suggestion https://github.com/bgruening/galaxytools/pull/1326#discussion_r1314559091, ie set profile and no detect_errors

[fubar2]

ok. one moment please....

[bernt-matthias]

I don't see in your coffee the possibility to omit the two attributes from the generated xml

[fubar2]

ok Thanks again @bernt-matthias. Default is None for detect_errors now. import_xml.xml now has profile and no detect_errors. It parses with testimport.py as expected giving 22.05 profile and an empty command tag now. Unit tests all pass with tox.

ross@pn50:~/rossgit/galaxyxml/test$ python testimport.py import_xml.xml
reading from import_xml.xml             
INFO:galaxyxml.tool.import_xml:<description> is loaded during initiation of the object.
INFO:galaxyxml.tool.import_xml:<stdio> loaded.                     
INFO:galaxyxml.tool.import_xml:<version_command> is loaded during initiation of the object.
<tool name="Import" id="import_test" version="1.0" profile="22.05">
  <description>description</description>                                                  
  <edam_operations>                      
    <edam_operation>operation_0004</edam_operation>                     
  </edam_operations>
  <edam_topics>
    <edam_topic>topic_0003</edam_topic>
  </edam_topics>                                                                          
  <requirements>     
    <requirement version="1" type="package">magic_package</requirement>
    <container type="docker">a_container</container>
  </requirements>
  <stdio>                                                                                 
    <exit_code range="1:" level="fatal"/>
  </stdio> 
  <version_command><![CDATA[v_command]]></version_command> 
  <command><![CDATA[                                                                                                                                                                
command interval_file $interval_file
$bool       
int_size $int_size
float_size $float_size

[fubar2]

I don't see in your coffee the possibility to omit the two attributes from the generated xml

Thanks again, @bernt-matthias but if you remove profile from the sample, it parses without a problem, and detect errors is also not there or in the output - so missing in the input is harmless. However, the emitted xml has what was supplied or the default profile if there was not one, satisfying https://galaxy-iuc-standards.readthedocs.io/en/latest/best_practices/tool_xml.html?highlight=profile#tool-profile and the rule that one should be forgiving of what is ingested but strict in what is emitted.

Are there situations where that guideline should not be followed in generating XML? If we are not following those guidelines, it's going to be hard to make a good decision.

[fubar2]

@hexylena @bernt-matthias: please let me know what outstanding issues remain that block this from being merged?

[hexylena]

However, the emitted xml has what was supplied or the default profile if there was not one, satisfying https://galaxy-iuc-standards.readthedocs.io/en/latest/best_practices/tool_xml.html?highlight=profile#tool-profile and the rule that one should be forgiving of what is ingested but strict in what is emitted.

This library isn't intended to generate IUC best practice tools, that's not it's goal, it's intended to let you flexibly generate the tool you need.

Are there situations where that guideline should not be followed in generating XML? If we are not following those guidelines, it's going to be hard to make a good decision.

this is a prescriptivist take (these are our rules and they are the only right way) and this library is targetted at people that know what they want to do and want flexibility, not enforcing IUC best practices (as you'll note, it's not under galaxy-iuc)

[hexylena]

@bernt-matthias you have most of the same review comments I did :sweat_smile: I should've hit submit earlier

[hexylena]

I would recommand also that don't make PRs from your master branch in the future, it's best to make them from a feature branch.

[hexylena]

@fubar2 @bernt-matthias

        python examples/example.py > tmp.xml
        xmldiff tmp.xml examples/tool.xml
        python examples/example_macros.py > tmp.xml
        xmldiff tmp.xml examples/example_macros.xml

I fixed the first set, I'm still struggling to figure out why the second set changed, @fubar2 maybe you can fix that.

[fubar2]

ok - thanks for the comments - appreciated.

Most problems are from changes to make <command> an XMLParam. Please let me know if you really want to leave them in this PR - would prefer to revert them otherwise.

Apologies for the import sample changes - from using it to confirm that either missing or supplied values were handled - shouldn't have committed those. Yes, too much at once and yes, a branch next time after this reminder... I'll try to figure the rest out and make adjustments in response after I revert the command stuff?

[hexylena]

It happens, no worries. Let's not dwell.

• Please feel free to update this PR accordingly, I've already pushed some changes to this branch :)
• Making it a proper Command is a good change and one that I would like to see kept, as I see it that's one of the core improvements in this PR.
• If the tests pass, well, that's what's really important in the end.

[fubar2]

ok. Thanks again!

[fubar2]

However, the emitted xml has what was supplied or the default profile if there was not one, satisfying https://galaxy-iuc-standards.readthedocs.io/en/latest/best_practices/tool_xml.html?highlight=profile#tool-profile and the rule that one should be forgiving of what is ingested but strict in what is emitted.

This library isn't intended to generate IUC best practice tools, that's not it's goal, it's intended to let you flexibly generate the tool you need.

Are there situations where that guideline should not be followed in generating XML? If we are not following those guidelines, it's going to be hard to make a good decision.

this is a prescriptivist take (these are our rules and they are the only right way) and this library is targetted at people that know what they want to do and want flexibility, not enforcing IUC best practices (as you'll note, it's not under galaxy-iuc)

Apologies - my immediate problem is to get things past review elsewhere - sorry it spilled over where it does not belong. I'll advocate for the changed defaults recommended by those reviewers, but all those decisions are entirely yours...

[fubar2]

I hope all the worst is gone and it's ready for another review. Reformatting is from running tests in the dumbest possible way so not intended but effective. Sorry for the noise. Defaults for profile and command detect_errors are None. The IUC guidance seems way out of date and hard to know which bits to take seriously. Disappointing and hard to support really. A condign punishment.

[hexylena]

The IUC guidance seems way out of date and hard to know which bits to take seriously. Disappointing and hard to support really. A condign punishment.

It would be great to get it updated but, things move very very slowly there it seems. c.f. my indentation PR allowing tabs, which is now dormant since months. We also didn't have the 'traditional' IUC meeting at GCC too, so, didn't have the usual going through of issues/etc.

[fubar2]

github action flake now unhappy so format issues to resolve or silence?

[hexylena]

Feel free to ignore flake. I've pushed a commit to take care of one test, then there's only one more failure:

12c12,13
<   <token name="ARAGORN_OUTMACRO"><![CDATA[]]></token>
---
>   <token name="ARAGORN_OUTMACRO"><![CDATA[-output '$output'
> ## TODO CLI for OutputCollection collection]]></token>

Previously the outmacro had included an -output '$output' with a TODO for the collection, now that seems to be missing, but I think this was an issue already for a while. You contributed the collection support ages ago (awesome thank you :yellow_heart:) but I think our tests haven't been working since then so we didn't notice until now.

So I will merge over this specific failure, and maybe if you have energy and bandwidth, if you have a nice fix for it, i think it would be useful to the library's consumers to retain this todo/-output

flake rant

It's stuff black will take care of which is especially, especially frustrating for me. Why lint and make a human fix it, just fix it and shut up (I say to flake/black.) These ridiculous tools should either fix it automatically because it's bloody whitespace, or stop complaining. `isort` is banned from all of my projects because it drives me up a wall.

[hexylena]

Thanks for the patience @fubar2 and putting up with our complaints. I'll mint a new release for ya

[hexylena]

after a silly amoutn of fighting with pypi/gha, https://pypi.org/project/galaxyxml/0.5.3/ now exists.