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

fubar2 · 2023-09-06T12:24:47Z

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!

Only way I could get it to work was to make command an XMLParam and add the command string as an etree.CDATA. It passes tests including a new one for the command but I'm not sure if this is the most efficient way to achieve this goal.

why not?

bernt-matthias

Good idea to allow setting a tool's profile and a command's detect_errors property.
Is setting detect_errors to anything else than "aggressive" supported?
Can you add a test showing how to set non-default values?

hexylena · 2023-09-06T15:02:39Z

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 · 2023-09-06T15:08:29Z

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

bernt-matthias · 2023-09-06T18:38:31Z

@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 · 2023-09-06T22:55:49Z

Thanks @hexylena and @bernt-matthias!

Yes, defaults are both what @bgruening asked for in bgruening/galaxytools#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?

That old problem of the galaxy utility libs being stuck at 23.05 is biting. I wonder when they will be updated and all the downstream packages? ` File "/home/ross/rossgit/galaxyxml/.venv/lib/python3.10/site-packages/galaxy_tool_util-23.0.5-py3.10.egg/galaxy/tool_util/deps/conda_util.py", line 90, in CondaContext _conda_version: Optional[Union[packaging.version.Version, packaging.version.LegacyVersion]] AttributeError: module 'packaging.version' has no attribute 'LegacyVersion' `

fubar2 · 2023-09-07T00:12:59Z

@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")

…t error user set values

bernt-matthias · 2023-09-07T05:31:08Z

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 · 2023-09-07T05:41:53Z

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 · 2023-09-07T05:55:55Z

For your PRs I would suggest to go with marius' suggestion bgruening/galaxytools#1326 (comment), ie set profile and no detect_errors

fubar2 · 2023-09-07T05:57:16Z

ok. one moment please....

bernt-matthias · 2023-09-07T05:59:14Z

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

seems just fine.

fubar2 · 2023-09-07T06:17:39Z

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 · 2023-09-07T06:33:44Z

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 · 2023-09-12T22:39:05Z

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

hexylena · 2023-09-13T08:15:02Z

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)

bernt-matthias

I think there are now some diffs in the example:

        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

Can you fix those?

galaxyxml/tool/import_xml.py

bernt-matthias · 2023-09-13T07:47:44Z

test/unit_test_import_xml.py

+        try:
+            de = self.tool.command.node.attrib["detect_errors"]
+        except KeyError:
+                de = None


Fix indentation here.

bernt-matthias · 2023-09-13T07:49:48Z

test/unit_test_import_xml.py

+        self.tool.command.node.attrib["detect_errors"] = "foobarfoo"
+        de = self.tool.command.node.attrib["detect_errors"]
+        self.assertEqual(de, "foobarfoo")


I don't understand this test. Looks like:

A = "foobarfoo" B = A self.assertEqual(B, "foobarfoo")

Same for the following two lines.

yes. Makes sure they are persistent and mutable after the previous test checks the correct values have been extracted from the sample. Was trying to respond to a previous suggestion. Will remove it.

bernt-matthias · 2023-09-13T07:50:54Z

test/import_xml.xml

@@ -1,5 +1,5 @@
 <?xml version="1.0"?>
-<tool id="import_test" name="Import" version="1.0">


The xml examplifies the defaults, or?

hexylena · 2023-09-13T09:17:42Z

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

hexylena

As of now it doesn't pass the tests (the other tests, in the .travis.yml that we didn't finish turning into a GHA.). I will update those @fubar2 .

$ diff tmp.xml examples/tool.xml 
1c1
< <tool name="aragorn" id="se.lu.mbioekol.mbio-serv2.aragorn" version="1.2.36" profile="22.05">
---
> <tool name="aragorn" id="se.lu.mbioekol.mbio-serv2.aragorn" version="1.2.36">
7a8,11
>   <stdio>
>     <exit_code range="1:" level="fatal"/>
>   </stdio>
>   <expand macro="stdio"/>
9,10c13
<   <command><![CDATA[
< aragorn.exe $flag
---
>   <command><![CDATA[aragorn.exe $flag

and yeah, fine with dropping the stdio, but still not crazy about adding profile by default, that really seems like something for library consumers to do. I also don't understand the newline being added there.

hexylena · 2023-09-13T08:30:24Z

galaxyxml/tool/__init__.py

-            command_line = self.command_override
+            command_text = self.command_override


why did you rename this?

Plan to revert all this - part of the command attributes change.
Took me a while to get my head around where the actual command text was being found in the new XMLParam and I discovered I was reusing a variable name. Went the wrong way - sorry - should have backed out but forged ahead with new names. Will soon be back to what it was.

The stdio default insertion is dropped.
Stdios still work fine if you want to use them. :)
This all started when I was asked to add profile and remove that deprecated default stdio value, in a review of some tools generated using galaxyxml, so not sure what else I can do here.

hexylena · 2023-09-13T08:31:37Z

galaxyxml/tool/import_xml.py

-        <command> is already loaded during initiation.
-
-        :param tool: Tool object from galaxyxml.
-        :type tool: :class:`galaxyxml.tool.Tool`
-        :param desc_root: root of <command> tag.
-        :type desc_root: :class:`xml.etree._Element`
+        now an XMLParameter with a text


please don't delete documentation, update it if it needs updating.

galaxyxml/__init__.py

hexylena · 2023-09-13T08:36:28Z

galaxyxml/tool/__init__.py

-        # Steal interpreter from kwargs
-        command_kwargs = {}
-        if export_xml.interpreter is not None:
-            command_kwargs["interpreter"] = export_xml.interpreter


yes, ok, this should not have been included. good. https://docs.galaxyproject.org/en/latest/dev/schema.html#id22 @bernt-matthias the documentation looks a bit odd there.

hexylena · 2023-09-13T08:37:39Z

galaxyxml/tool/__init__.py

+            if getattr(self, "command_line", None):
+                ctext = export_xml.command_text


now that you've renamed command_line, this should be command_text, right?

Suggested change

if getattr(self, "command_line", None):

ctext = export_xml.command_text

if getattr(self, "command_text", None):

ctext = export_xml.command_text

hexylena · 2023-09-13T08:47:14Z

galaxyxml/tool/__init__.py

+                )
+            ctext = actual_cli.strip()
+        export_xml.command_text = ctext
+        ctext = '\n' + ctext # pretty - bjoern's suggestion


I don't love this tbh, it'll introduce lots of diffs to add some useless whitespace. If you want a newline in your command add it, if you want it pretty, run it through planemo. This isn't the job of this library

hexylena · 2023-09-13T09:04:23Z

tox.ini

+[testenv]
+description = run the tests with pytest
+package = wheel
+wheel_build_env = .pkg
+deps =
+    pytest>=6
+commands =
+    pytest test/unit_test_import_xml.py {tty:--color=yes} {posargs}


We have not been using tox to test, this is a bigger change, hmm. until now it was all nose, which apparently is now broken. I really wish some of these changes had come in separate PRs so we could've discussed them in isolation, but I guess we have to accept this because nose is non functional.

galaxyxml/tool/import_xml.py

hexylena · 2023-09-13T09:21:51Z

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 · 2023-09-13T09:22:52Z

@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 · 2023-09-13T10:08:17Z

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 · 2023-09-13T10:36:13Z

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 · 2023-09-13T10:46:17Z

ok.
Thanks again!

fubar2 · 2023-09-18T10:05:33Z

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 · 2023-09-19T05:02:14Z

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 · 2023-09-19T09:18:10Z

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.

hexylena

This round was much better to review, thanks for changes @fubar2.

…ds to start with test for tox to run automagically it seems. Reverted the sample

fubar2 · 2023-09-21T11:46:37Z

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

hexylena · 2023-09-22T07:21:48Z

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 💛) 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 · 2023-09-22T07:27:19Z

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

hexylena · 2023-09-22T08:10:41Z

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

fubar2 added 3 commits September 6, 2023 20:17

tox ini does the needful and runs the unit tests

0139b27

why not?

cleanup. is good. is xml.

eb30341

bernt-matthias reviewed Sep 6, 2023

View reviewed changes

fubar2 mentioned this pull request Sep 6, 2023

ampir Panaroo and Yara ToolFactory generated tools. bgruening/galaxytools#1326

Closed

add another unit test to demonstrate mutability for profile and detec…

7d36101

…t error user set values

changed default to just profile. No detect_errors.

b55f3bf

seems just fine.

bernt-matthias reviewed Sep 13, 2023

View reviewed changes

hexylena added 3 commits September 13, 2023 10:50

Not the job of this library

a4ae18e

Ensure tests pass

cf071aa

cleanup tox, pass tests

db41171

hexylena reviewed Sep 13, 2023

View reviewed changes

cleanup the tox

4b974f3

fubar2 added 5 commits September 18, 2023 14:56

trying to undo lots of overdo.

98d0728

not saved..

c57b26d

reformat during tests...

4825ce0

get rid of notional test..

5419d67

revert order in init

683b133

eeesh. Found a bad refactor booboo.

9a7af55

fubar2 requested review from hexylena and bernt-matthias September 19, 2023 04:55

hexylena approved these changes Sep 19, 2023

View reviewed changes

fubar2 added 3 commits September 21, 2023 15:48

renamed unit test back to unit_test.. but that stops it testing - nee…

d7d3d3c

…ds to start with test for tox to run automagically it seems. Reverted the sample

reformat

3cd2c22

mostly my own flake things

224de6f

remove stdio from ref

a9dcc20

whatever

cab0cb3

hexylena approved these changes Sep 22, 2023

View reviewed changes

hexylena merged commit 72c6921 into hexylena:master Sep 22, 2023
0 of 2 checks passed

		@@ -1,5 +1,5 @@
		<?xml version="1.0"?>
		<tool id="import_test" name="Import" version="1.0">

		command_line = self.command_override
		command_text = self.command_override

		if getattr(self, "command_line", None):
		ctext = export_xml.command_text

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

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

Conversation

fubar2 commented Sep 6, 2023 • edited Loading

bernt-matthias left a comment

Choose a reason for hiding this comment

hexylena commented Sep 6, 2023 • edited Loading

hexylena commented Sep 6, 2023

bernt-matthias commented Sep 6, 2023

fubar2 commented Sep 6, 2023 • edited Loading

fubar2 commented Sep 7, 2023 • edited Loading

bernt-matthias commented Sep 7, 2023

fubar2 commented Sep 7, 2023 • edited Loading

bernt-matthias commented Sep 7, 2023

fubar2 commented Sep 7, 2023

bernt-matthias commented Sep 7, 2023

fubar2 commented Sep 7, 2023 • edited Loading

fubar2 commented Sep 7, 2023 • edited Loading

fubar2 commented Sep 12, 2023 • edited Loading

hexylena commented Sep 13, 2023

bernt-matthias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexylena commented Sep 13, 2023

hexylena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexylena commented Sep 13, 2023

hexylena commented Sep 13, 2023

fubar2 commented Sep 13, 2023 • edited Loading

hexylena commented Sep 13, 2023 • edited Loading

fubar2 commented Sep 13, 2023

fubar2 commented Sep 18, 2023

fubar2 commented Sep 19, 2023 • edited Loading

hexylena commented Sep 19, 2023

hexylena left a comment

Choose a reason for hiding this comment

fubar2 commented Sep 21, 2023

hexylena commented Sep 22, 2023

hexylena commented Sep 22, 2023

hexylena commented Sep 22, 2023

fubar2 commented Sep 6, 2023 •

edited

Loading

hexylena commented Sep 6, 2023 •

edited

Loading

fubar2 commented Sep 6, 2023 •

edited

Loading

fubar2 commented Sep 7, 2023 •

edited

Loading

fubar2 commented Sep 7, 2023 •

edited

Loading

fubar2 commented Sep 7, 2023 •

edited

Loading

fubar2 commented Sep 7, 2023 •

edited

Loading

fubar2 commented Sep 12, 2023 •

edited

Loading

fubar2 commented Sep 13, 2023 •

edited

Loading

hexylena commented Sep 13, 2023 •

edited

Loading

fubar2 commented Sep 19, 2023 •

edited

Loading