Drop lint from preventing deployment

[mvdbeek]

and we should really take a good look at the signal we're sending by applying linting rules to PRs for things that are already broken. It does increase the barrier to contribution significantly and leads to PRs that do more than one thing and that's bad IMO.

FOR CONTRIBUTOR:

• - I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
• - License permits unrestricted use (educational + commercial)
• - This PR adds a new tool or tool collection
• - This PR updates an existing tool or tool collection
• - This PR does something else (explain below)

[mvdbeek]

What we do in the Galaxy code base is fix whatever new linting rule (think eslint, ruff, black) we're applying before making it a requirement, we should follow that same principle here.

[bernt-matthias]

I understand the frustration, but I think we need a better solution.

But in my opinion, this change is just fixing symptoms. If there is an error in the linter then it should be fixed upstream instead of disabling linting (note that we already skip linter warnings - which is fine). If the linter is correct then we may need to pin the linter version, i.e. planemo (?), in order to have control over the linter (code/version) that is applied .. and IUC has the possibility to fix the code before updating the pin. Maybe (self-critically) we also need a better review (or more rigorous testing) of changes made in the linting code.

and we should really take a good look at the signal we're sending by applying linting rules to PRs for things that are already broken.

Ideally yes :)

My perception is that IUC does not have the resources or interest (obviously each of us is interested only in a subset of the tools and cares more about those than others) in fixing existing problems. An example of the lack of resources is that there haven't been successful weekly CI tool tests for a very long time (and with your argumentation, we could also disable tool tests for deployment).

For linting there are numerous issues systematically listing linter warnings and other problems (#4100, #4094, #4093, #4091, #4085, #4009) that did not receive much attention. I remember that I once wanted to add linting to CI, but it was noted that this creates too much noise, i.e. at the moment we don't even know what is broken wrt. linting.

So, in my opinion, we need any help we can get. Often linter warnings are very easy to fix and people who open PRs for tools obviously care for them. So why shouldn't we point them to other things that could be fixed and kindly ask/nudge them to do so? In the end, they don't have to.

leads to PRs that do more than one thing and that's bad IMO.

Would be fine for me as long as the commits are atomic.

[mvdbeek]

I understand the frustration, but I think we need a better solution.

Yes, see #5650 (comment).

My perception is that IUC does not have the resources or interest (obviously each of us is interested only in a subset of the tools and cares more about those than others) in fixing existing problems

Not every IUC member needs to care about all tools, but I know you and I are not alone in this, to that non-exhaustive list I would add at least @nsoranzo and @bgruening (apologies if I missed anyone there). This is the reason for my continued involvement here, I otherwise really don't care about any particular tool. Now another question is the timeline of introducing major changes affecting contributors.
You can't enable a whole barrage of things and then push it onto someone just fixing a bug in an existing tool. That's not how any project that I am aware of handles this.

So why shouldn't we point them to other things that could be fixed and kindly ask/nudge them to do so?

Because that's how you make it seem that contributing here is always a huge effort. As you said that:

linter warnings are very easy to fix

... for us

We sure have the resources to fix all linting errors on a per rule basis, but that needs to be coordinated and prioritized.

Would be fine for me as long as the commits are atomic.

it is not fine for me at all, and it's quite problematic when every bugfix potentially comes with a new bug. atomicity means nothing if you can't install those commits.

#4100

enforcing this is IMHO a waste of our time, it has zero impact and is the last thing I would do after everything important has been addressed. I hope that's not anything that currently fails the CI.

#4094

hard for me to see why that's important but I give you that it may hide some bugs

#4093

that seems important, but do we lint for this ?

#4091

unclear how that's supposed to be fixed and why it is a problem. Is there a linter for it ?

Would you be onboard with this plan:

• roll back any warning level failures for everything that currently fails
• prioritize the rules we want to fix
• fix the tools for that new rule
• make violations to the rule a CI error

[bernt-matthias]

#4093

that seems important, but do we lint for this ?

Yes, https://github.com/galaxyproject/galaxy/blob/1ed254ff72b1c6c4399e0d47dbc7716bc68ff01c/lib/galaxy/tool_util/linters/outputs.py#L50 .. but super hard to fix since it affects mothur which fails tests for ages.

Would you be on board with this plan:

* roll back any warning level failures for everything that currently fails

I only would like to do this for linter messages that are not valid, i.e. false positives of the linter. Or even better just fix the linter if possible. For me, the problem with rolling back all of them is

1. the problem is to large. Out of the linter messages we have in the galaxy code base the tools in IUC trigger

• approx 1/3 of the unique error messages
• more than half of the unique warning messages

2. it affects all tool repositories and in particular the language server. If we would disable all those warnings tool development quality might suffer quite a bit.

We certainly have to work on this. My suggestion for the meantime would be:

• revert to use --error-level error instead of warn in the PR workflow (to get a better contributor experience .. you certainly have a good point here)
• add a linter job to weekly CI (to create awareness and track our progress)

* prioritize the rules we want to fix
* fix the tools for that new rule

This part I fully support. For a start, I just ran shed_lint to get an overview of the extent of the problem. At the moment we have

• 157 linter errors shed_lint_error.txt corresponding to 21 unique messages
• 1580 linter warning shed_lint_warn.txt corresponding to 35 unique messages

If I'm not wrong we have a total of 53 unique warning messages in our linters and 76 error messages (determined by just egrep "error|(warn(" lib/galaxy/tool_util/linters/*py).

Wondering how to discuss/prioritize those most efficiently. I could imagine a shared document that everyone can reorder/comment on might be more useful than a GitHub issue, or? I'm also sure some of the problems can also be solved by automatic means.

I think what we could need on the long run is a code for each linter message (like the pep8/flake codes), such that the repos / users can select which rules should apply. Maybe also restructure the linter code, such that we have 1 linter function per message.

[mvdbeek]

but super hard to fix since it affects mothur which fails tests for ages.

[noci] seems fine in that case ?

the problem is to large. Out of the linter messages we have in the galaxy code base the tools in IUC trigger

I do not understand the argument. Take one rule, fix it, done. There isn't really any viable alternative. Noone is going to fix all issues in a single go. And if they did it'd be practically unreviewable.

2. it affects all tool repositories and in particular the language server

Exactly! We should not force this on people, certainly for things that are of no consequence. Now I think the language server is the right place to show these messages, and it can be more strict than we are here.

• 157 linter errors shed_lint_error.txt corresponding to 21 unique messages
• 1580 linter warning shed_lint_warn.txt corresponding to 35 unique messages

Thanks for that. Let's start by redefining what errors are:

     30 .. ERROR: Test ?: Test param ... not found in the inputs
     28 .. ERROR: Error '404 Client Error
     24 .. ERROR: Data parameter [...] filter needs to define a ref attribute
     21 .. ERROR: Data parameter [...] for filters only type="data_meta" is allowed, found type="add_value"
     11 .. ERROR: Tool defines multiple parameters with the same name:
     11 .. ERROR: some HTTPConnectionPool error
     10 .. ERROR: Invalid XML found in file:
      4 .. ERROR: Tool output [...] has duplicated name
      3 .. ERROR: Error '403 Client Error
      3 .. ERROR: Data parameter [...] for filters only type="data_meta" and key="dbkey" are allowed, found type="add_value" and key="None"
      2 .. ERROR: Select parameter [...] has multiple options with the same value
      2 .. ERROR: Select parameter [...] has multiple options with the same text content
      1 .. ERROR: Tool defines an output with a name equal to the name of an input: 'log'
      1 .. ERROR: Test 4: Cannot make assumptions on the number of outputs in a test expecting failure.
      1 .. ERROR: Test 3: test output 'outfile' must have a 'count' attribute and/or 'discovered_dataset' children
      1 .. ERROR: Test 3: Attribute lines_diff is incompatible with compare="sim_size".
      1 .. ERROR: Test 2: unknown attribute 'size' for 'has_size'
      1 .. ERROR: Test 2: test output 'outfile' must have a 'count' attribute and/or 'discovered_dataset' children
      1 .. ERROR: Test 2: 'has_size' needs to specify 'value', 'min', or 'max'
      1 .. ERROR: Test 1: test output 'outfile' must have a 'count' attribute and/or 'discovered_dataset' children
      1 .. ERROR: Test 12: Test param totReq not found in the inputs

Only two items here would qualify as an actual error in my book, 1 .. ERROR: Tool defines an output with a name equal to the name of an input: 'log'. And that's also only true if the param hierarchy is taken into account.
And the second is 10 .. ERROR: Invalid XML found in file:

The rest seem like warnings at best.

I think what we could need on the long run is a code for each linter message (like the pep8/flake codes), such that the repos / users can select which rules should apply.

This cannot be "the long run", this must happen before we can reasonably clean up large categories, like the warning linters. I was under the impression that we can do that already with --skip, is that not the case ?

[bernt-matthias]

[noci] seems fine in that case ?

I agree, but wrt your point on atomic comits / PRs this also would result in a non-installable intermediate tool version. But this is certainly fine for me.

I do not understand the argument. Take one rule, fix it, done. There isn't really any viable alternative

The question is what you meant by "roll back any warning level failures ". I understood to remove / disable the linter rules, i.e. in tool-util. I'm totally fine with fixing all this :)

Only two items here would qualify as an actual error in my book

Those two are certainly of the highest priority. My problem is that we do not have a definition of what an error is. I guess that your definition of an error is everything that makes the tool run fail. My definition would be more strict: everything that makes any part of the tool fail would qualify as an error, e.g. filter needs to define a ref attribute renders the filter dysfunctional, I would also consider things that invalidate tests as an error.

But this discussion in large parts can be avoided if we have the possibility for a more fine grained selection.

this must happen before we can reasonably clean up large categories, like the warning linters.

+1

I guess, we can just add a linter code here and think of a naming scheme. Could be just numbers like for PEP or a few capital letters that identify each message. Then restructure the code such that each error/warning code corresponds to one linter class. Some of the linters might even have a fix() method that could auto-fix the problems :)

I was under the impression that we can do that already with --skip, is that not the case ?

We can only skip whole linters, e.g. the inputs linter, but not separate linter messages. When we restructured the linter code we should be able to do this with the current planemo code.

Would my plan for the meantime be acceptable for you, i.e. use error as fail level and install weekly linter CI?

[mvdbeek]

The question is what you meant by "roll back any warning level failures ".

our fail level is now warning for PRs (https://github.com/galaxyproject/tools-iuc/blob/main/.github/workflows/pr.yaml#L132), this isn't reasonable currently. We can either keep it at warning and skip the ones that are currently not fixed across tools-iuc, or move it back to error. If that isn't visible enough we can use the planemo lint >> $GITHUB_STEP_SUMMARY and list this as things to check for reviewers.

But this discussion in large parts can be avoided if we have the possibility for a more fine grained selection.

yes, I agree, that's a very valuable thing to do.

[bernt-matthias]

yes, I agree, that's a very valuable thing to do.

I started .. open for discussion galaxyproject/galaxy#17081

[pcm32]

I think that it would be very good that this keeps moving forward, I very often these days think twice on whether to make changes to old tools because of the effort it involves to please the linter on so many things that are irrelevant for the tool to keep working as it has been so far.

[bernt-matthias]

@pcm32 I think way forward will be the possibility to enable/disable linter rules in planemo. This will be possible soonish. The ingredients are

• Split linters in separate classes galaxy#17081
• Tool linting add --skip_file and remove lxml schema validation planemo#1420 (requires Bump galaxy packages to allow for 24.0 planemo#1441)

Then we can disable all rules that are not fulfilled repo-wide (and systematically work on them).

@mvdbeek could you find the time to do a new release (or pre replease) of the python packages for 24.0 such that we can progress on planemo?

[mvdbeek]

Newer than https://pypi.org/project/galaxy-tool-util/24.0.0/ ?

[bernt-matthias]

I remember that most of the problems in the planemo 24.0 PR should be fixed in galaxyproject/galaxy#17899

I was assuming that we need new packages to fix this in the PR. Am I wrong?

Is there a way to speed this up (e.g. by having a conditional requirement in planemo's requirement.txt that installs galaxy packages from git if in CI)?

[mvdbeek]

I've created a new minor release, should have new packages once https://github.com/galaxyproject/galaxy/actions/runs/8924916026/job/24512355542 completes.

[mvdbeek]

And yes, you can also install from git, pip install 'git+https://github.com/galaxyproject/galaxy.git@release_24.0#egg=galaxy-tool-util&subdirectory=packages/tool_util'

[bernt-matthias]

#6184