Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[23.0] Allow duplicate labels in linter if outputs contain filters #15933

Merged
merged 4 commits into from
Jul 25, 2023
Merged

[23.0] Allow duplicate labels in linter if outputs contain filters #15933

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
af797d9
fix linter: duplicated label check
bernt-matthias Apr 13, 2023
2775005
make duplicated label a warning
bernt-matthias May 9, 2023
c684e0b
also warn in case of a filter
bernt-matthias May 9, 2023
da8fc06
more black
bernt-matthias Jul 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib/galaxy/tool_util/linters/outputs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,14 @@ def lint_output(tool_xml, lint_ctx):

label = output.attrib.get("label", "${tool.name} on ${on_string}")
if label in labels:
lint_ctx.error(f"Tool output [{name}] uses duplicated label '{label}'", node=output)
filter_node = output.find(".//filter")
if filter_node is not None:
lint_ctx.warn(
f"Tool output [{name}] uses duplicated label '{label}', double check if filters imply disjoint cases",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What case triggered this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had it here: galaxyproject/tools-iuc#5291 .. but found it acceptable in this case to use different labels. Can't remember a really good example.

Point is that we (probably?) need to allow for the possibility to have two outputs that are never outputed together (i.e. they use disjoint filters). Then it might OK to allow for the same label. For instance if from_work_dir needs to be different when a flag is set or not set.

Since it seemed pretty hard (or impossible) to check the filters if they are disjoint I thought that its best to just check if there is a filter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it might OK to allow for the same label. For instance if from_work_dir needs to be different when a flag is set or not set.

I'd move the output to the right place or use the same output name, which is much friendlier for predicting what outputs will be generated. I think a warning is just the right level here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make this a warning then the outcome (IUC wise) would be the same (galaxyproject/tools-iuc#5260), ie. the PR workflow would fail.

But, given your idea to skip linting for the push to master/main we could just raise the fail level to error for the merge.

Just tell me your preference (warning / info) and I will change it.

Copy link
Member

@mvdbeek mvdbeek May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning is the right level IMO, galaxyproject/tools-iuc#5291 was improved by the new label and I think in many / all instances there are tweaks that would make the tool better as in either more userfriendly or more predictable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this example @mvdbeek: galaxyproteomics/tools-galaxyp#721 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, can you point out what I should look at there ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an output dataset and an output collection which are mutually exclusive. They have the same name which I find acceptable in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so you're also saying it should be a warning ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, according to my intuition of the meaning of "warning".

node=output
bernt-matthias marked this conversation as resolved.
Show resolved Hide resolved
)
else:
lint_ctx.warn(f"Tool output [{name}] uses duplicated label '{label}'", node=output)
labels.add(label)

format_set = False
Expand Down
18 changes: 14 additions & 4 deletions test/unit/tool_util/test_tool_linters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,12 @@
<outputs>
<data name="valid_name" format="fasta"/>
<data name="valid_name" format="fasta"/>
<data name="another_valid_name" format="fasta" label="same label may be OK if there is a filter">
<filter>a condition</filter>
</data>
<data name="yet_another_valid_name" format="fasta" label="same label may be OK if there is a filter">
<filter>another condition</filter>
</data>
</outputs>
</tool>
"""
Expand Down Expand Up @@ -1492,13 +1498,17 @@ def test_outputs_discover_tool_provided_metadata(lint_ctx):
def test_outputs_duplicated_name_label(lint_ctx):
tool_source = get_xml_tool_source(OUTPUTS_DUPLICATED_NAME_LABEL)
run_lint(lint_ctx, outputs.lint_output, tool_source)
assert "2 outputs found." in lint_ctx.info_messages
assert "4 outputs found." in lint_ctx.info_messages
assert len(lint_ctx.info_messages) == 1
assert not lint_ctx.valid_messages
assert not lint_ctx.warn_messages
assert len(lint_ctx.warn_messages) == 2
assert "Tool output [valid_name] uses duplicated label '${tool.name} on ${on_string}'" in lint_ctx.warn_messages
assert (
"Tool output [yet_another_valid_name] uses duplicated label 'same label may be OK if there is a filter', double check if filters imply disjoint cases"
in lint_ctx.warn_messages
)
assert "Tool output [valid_name] has duplicated name" in lint_ctx.error_messages
assert "Tool output [valid_name] uses duplicated label '${tool.name} on ${on_string}'" in lint_ctx.error_messages
assert len(lint_ctx.error_messages) == 2
assert len(lint_ctx.error_messages) == 1


def test_stdio_default_for_default_profile(lint_ctx):
Expand Down