Skip to content

Fix QA group dispatch: pass QA via the qa kwarg, not groups#156

Merged
ChrisRackauckas merged 4 commits into
SciML:masterfrom
ChrisRackauckas-Claude:fix-qa-group-dispatch
Jun 20, 2026
Merged

Fix QA group dispatch: pass QA via the qa kwarg, not groups#156
ChrisRackauckas merged 4 commits into
SciML:masterfrom
ChrisRackauckas-Claude:fix-qa-group-dispatch

Conversation

@ChrisRackauckas-Claude

@ChrisRackauckas-Claude ChrisRackauckas-Claude commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Problem

The QA CI job fails on master (introduced by the canonicalization conversion #154) before any test runs:

ERROR: LoadError: ArgumentError: run_tests: GROUP="QA" was requested but no `qa` body was provided
   @ SciMLTesting .../SciMLTesting.jl:959
   @ test/runtests.jl:3

The conversion placed the QA group inside the groups Dict. But SciMLTesting.run_tests routes GROUP="QA" through a dedicated elseif group == "QA" branch that consumes the qa keyword argument and short-circuits before the groups table is ever consulted. With qa left unset, that branch throws "no qa body was provided" — so a QA entry in groups is unreachable for GROUP="QA".

Fix

Move the QA spec from the groups Dict to the dedicated qa keyword argument (the documented SciMLTesting form). all = ["Core"] is retained, so QA still runs only when GROUP="QA" is explicitly selected and remains excluded from the default GROUP="All" / local ]test run, exactly as before.

Verification (local, Julia 1.10 + SciMLTesting v1.2.0)

Reproduced the exact CI error with the old groups["QA"] form, then confirmed the fix with a stub tree mirroring test/ (core.jl + qa/qa.jl + qa/Project.toml):

  • GROUP=QAexit 0, QA body runs (was: ArgumentError: no qa body provided)
  • GROUP=Core → exit 0, Core only
  • GROUP=All / no GROUP → exit 0, Core only (QA excluded)

In a checkout of this repo, GROUP=QA julia test/runtests.jl now reaches the QA branch, activates test/qa, develops local FEniCS, and resolves Aqua/JET/PyCall — it no longer throws the dispatch error (the local run then needs the cmhyett/julia-fenics Python stack, which is only present in CI).

Note: the other three master jobs (Core julia 1, Core julia pre, QA julia lts) failed separately with permission denied ... docker.sock — transient self-hosted-runner infrastructure errors unrelated to this change; not addressed here.

Please ignore until reviewed by @ChrisRackauckas.


Follow-up: fix the QA package-quality failures the QA group now surfaces (commits 85a709b, f70877a)

With the dispatch fixed, the QA group runs FEniCS's own Aqua.test_all/JET.report_package for the first time, exposing six pre-existing package-quality issues (all present on master, none introduced here). Each is fixed at its real cause; no Aqua check is disabled or loosened:

QA check Finding Fix
Piracy write(path, solution, time::Number) (jsolve.jl) extended Base.write with no FEniCS-owned argument type. Keep extending Base.write (so write stays a single function usable unqualified), but type solution::fenicsobject so the method is FEniCS-owned, not piracy. The unused, unavoidably-piratical write(::Any, ::PyObject, ::Number) overload is dropped (use store for raw vectors).
Method ambiguity The write method's untyped path clashed with Base.write(::IO, ...) and write(::AbstractString, ...). Type path::PyObject (what XDMFFile/TimeSeries return), disjoint from IO/AbstractString.
Undefined exports export fenicsclass named a binding that doesn't exist — the macro is @fenicsclass. export @fenicsclass.
JET (1 report) Argument(V, number)Argument(::Any, ::Any, ::Nothing) no matching method, because part::StringOrSymbol cannot hold its own nothing default. Widen to part::Union{StringOrSymbol, Nothing} = nothing (PyCall maps nothingNone, matching fenics.Argument(part=None)).
Stale dependencies ProgressMeter declared but unused (only a no-op @require ... using ProgressMeter; no function from it is ever called). Remove from [deps], [compat], and the dead __init__ block.
deps_compat LinearAlgebra (runtime stdlib dep) had no [compat] entry. Add LinearAlgebra = "1".

The first attempt (85a709b) instead removed write from the Base import, which made FEniCS.write clash with the exported Base.write and broke the Core examples (UndefVarError: write in tutorial8/9). f70877a corrects that by extending Base.write and disambiguating via argument types as above.

Verified locally in the cmhyett/julia-fenics container (the fenics Python stack) with Julia 1.10: Aqua.test_all(FEniCS) → 11 passed / 0 failed; JET.test_package(FEniCS) → 1 passed / 0 failed; and a module … using FEniCS; write(…) sanity check confirms the Base.write name clash is gone. (The earlier CI on 85a709b had already shown both QA legs green — i.e. piracy/exports/stale/deps_compat/JET all fixed — and Core red from the clash; f70877a keeps QA green and unbreaks Core.)

Please ignore until reviewed by @ChrisRackauckas.

ChrisRackauckas and others added 4 commits June 15, 2026 10:16
The canonicalization conversion (SciML#154) placed the QA group inside the
`groups` Dict, but SciMLTesting's `run_tests` routes `GROUP="QA"` through
a dedicated `elseif group == "QA"` branch that consumes the `qa` keyword
argument and short-circuits before the `groups` table is ever consulted.
With `qa` unset, that branch throws

    ArgumentError: run_tests: GROUP="QA" was requested but no `qa` body
    was provided

so the QA CI job failed on master before loading FEniCS. Moving the QA
spec to the `qa` kwarg (matching the documented SciMLTesting form) makes
`GROUP="QA"` reach the QA body, while `all = ["Core"]` keeps QA out of the
default `GROUP="All"`/local `]test` run as before.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Verify the scoped GitHub-hosted runner routing (SciML/.github SciML#97) now
sends this repo's apt-packages/container legs to ubuntu-24.04.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The QA group now actually runs FEniCS's own Aqua/JET checks, surfacing
six pre-existing package-quality issues. Fix the real causes:

- Type piracy + Base.write method ambiguity: `write(path, solution,
  time::Number)` extended `Base.write` with no FEniCS-owned argument
  type, which is piracy and also clashed with `Base.write(::IO, x1,
  xs...)`. Stop importing `write` from Base so `write`/`store` are
  FEniCS-owned functions (still exported); behavior unchanged.
- Undefined export: `export fenicsclass` referenced a nonexistent
  binding; the macro is `@fenicsclass`. Export the macro.
- JET no-matching-method: `Argument(V, number)` lowered to
  `Argument(::Any, ::Any, ::Nothing)` because `part::StringOrSymbol`
  cannot accept its own `nothing` default. Widen to
  `Union{StringOrSymbol, Nothing}`.
- Stale dependency: ProgressMeter was declared but unused (only a
  dead `@require ... using ProgressMeter` no-op). Drop it from deps,
  compat, and the no-op __init__ block.
- deps_compat: add a `[compat]` entry for the LinearAlgebra stdlib.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…g types

The prior commit removed `write` from the `Base` import to kill the type
piracy, but that made `FEniCS.write` a distinct exported binding that
clashed with the exported `Base.write` ("both FEniCS and Base export
write"), breaking the Core examples (tutorial8/9: `UndefVarError: write`).

Restore `import Base: ...write` so `write` stays a single function, and
fix the two QA findings on it through argument types instead:

- Type piracy: `write` now takes `solution::fenicsobject` (a FEniCS-owned
  type), so the method is owned by FEniCS rather than pirating Base. The
  unused, unavoidably-piratical `write(::Any, ::PyObject, ::Number)`
  overload is dropped (use `store` for raw vectors like `vector(u)`).
- Method ambiguity: type `path::PyObject` (what `XDMFFile`/`TimeSeries`
  return), which is disjoint from `Base.write(::IO, ...)` and
  `write(::AbstractString, ...)`, removing both ambiguities.

`store` is FEniCS-owned (not a Base function), so its overloads are not
piracy; `path::PyObject` is added there too for symmetry/correctness.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ChrisRackauckas ChrisRackauckas marked this pull request as ready for review June 20, 2026 11:05
@ChrisRackauckas ChrisRackauckas merged commit d0fdd29 into SciML:master Jun 20, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants