Self-review turned up three problems in the guidance I just added.
The mapped array idiom joined paths unquoted. shell_command is the one branch
that doesn't shlex.quote its result (unlike base_command/arguments right above
it in _build_command_line), so a path with a space split into two arguments.
Switched to the per-element quoted form our own test tool
cat_multiple_user_defined.yml uses, which renders cat '/d/s 1.fq' '/d/b.fq'.
"There is no [] indexing syntax" was too strong and simply wrong -- expressions
are JavaScript with InlineJavascriptRequirement always on, so
inputs.some_repeat[0].x is fine and the validator's own comment cites that form.
Only the empty [] is a syntax error. Narrowed the claim to that.
And scoping `value` to the types that have one: select and data forbid it and
fail with extra_forbidden, so telling the model to give every optional parameter
a `value` traded one validation failure for another. Select defaults are
selected: true on an option. Same correction on the critic side, where asking
for a field that can't exist would have set needs_full_refine and burned an
extra generation pass.
Also dropped the critic's unconditional claim that a dedicated step resolves the
container, which has the same off-by-default problem as the producer prompt.
A pass over the rest of the prompt against the actual model turned up several
places where it tells the model something that isn't true.
The container section promised Galaxy re-resolves a "close, plausible" image to
a verified biocontainer, but container_recommendation_enabled is off by
default, so the guess is usually what runs -- and the accuracy section right
below told the model never to guess. Reworded so the two agree.
`help` was described as help text when it wants a HelpContent object; a scalar
fails validation, so there's an example now. Collection outputs need
discover_datasets rather than from_work_dir, which the generic "one or the
other" wording hid. The id pattern is stricter than "lowercase with hyphens" --
it must start with a letter. And GALAXY_SLOTS was promised to equal cores_min,
which only holds where the deployment maps it; the local runner falls back to
local_slots or 1.
Also fixed the critic prompt, which asked for `default` on optional parameters.
Scalar inputs use `value` and forbid extras, so acting on that suggestion
produced a tool that fails validation.
The prompt told the model to reach for $(inputs.param_name[].path) on multiple
data inputs, which isn't valid JavaScript. Expressions go through do_eval, so
that form dies with "SyntaxError: Unexpected token ']'" -- and nothing catches
it up front, because _check_input_refs only extracts the leading input name. A
tool using it validates, saves, and then fails when Galaxy builds the command
line.
Swapped in the mapped form our own user_defined_tools docs use, and said
plainly that there's no [] indexing, since the old wording is exactly the shape
a model would pattern-match to. Prettier reindented the YAML examples it found
on the way past; those blocks parse identically before and after.
get_data_stream returns something that owns an open read, but its Iterator[bytes]
type said only that bytes come out of it. Callers had to probe for close() with
getattr to find out whether they were holding a connection.
Return a DataStream protocol instead, so the obligation to close an unconsumed
stream is part of the contract rather than a comment, and the service can just
close it.
cloudbridge picked the read size itself and it was never a tuning decision: 4 KiB
on AWS, inherited from a 2017 boto2 shim. A 10 GB download is 2.6 million reads
at that size, each one a threadpool hop on the way to the client.
4.4.0 takes a chunk_size, so pass the same STREAM_CHUNK_SIZE the other backends
stream at, and require that version where the dependency is declared.
`closing_stream` became a one-line factory when the context manager grew into a
class, so two names described one thing across four backend modules. Every
signature already says `RemoteDataStream`; the backends now build one directly.
`_ClosingIterator` wraps a generator this module creates, not an arbitrary
iterator, so say so and call `close()` outright rather than reaching for it with
`getattr`. The whole point of the surrounding change is that a chunk iterator
having a `close` proves nothing about what it releases -- worth not modelling the
one case where we do know.
Make remote data streams explicitly closable before their tee generator is entered, and have GalaxyStreamingResponse close synchronous response content on normal completion and failure.
Also release the stream if download logging or response construction fails, with regressions for all three pre-iteration paths.
#19255 reports three backends. Two are boto3 and this branch already fixes them.
The third, and the one the reporter called unusable, is generic_s3: 4GB+
downloads succeed but arrive truncated to about 3GB, silently.
generic_s3 is GenericS3ObjectStore, which extends the boto2 S3ObjectStore -- a
different class from the boto3 one that got `_stream_remote` -- so it inherited
the no-op and kept pulling into the cache. It is also the case the tee handles
best: `_tee_to_cache` compares streamed bytes against the store's size, turning a
silent truncation into a loud failure instead of a corrupt cached object.
boto2's Key reads in sized chunks like the boto3 body does. Its close reads the
rest of the response first unless told not to, hence fast=True: a client that
hung up should not make Galaxy drain 3GB before releasing the connection.
`_stream_remote` handed back a bare `Iterator[bytes]`, which has no way to
release what it is reading from. Closing botocore's `iter_chunks` only ends the
generator -- it is a plain loop over `StreamingBody.read` -- so the response
stays open and its connection never returns to the pool. Cancelled downloads are
the normal case on this path: it exists for the big transfers clients abandon.
Backends now return a context manager owning the read, built by `closing_stream`
from the chunks and the SDK's own close. `_tee_to_cache` enters it, so the
connection is released whether the client took every byte, hung up part way, or
the stream errored.
Also open the read last, in `_get_data_stream` and in `_stream_from_object_store`
alike. Both used to open first and could then decide against streaming -- an
unknown remote size, a size lookup that raised -- returning None with a live read
that nobody owned.
--buffer-size="$GALAXY_MEMORY_MB_PER_SLOT"M is a no-op on the default
deployment. MEMORY_STATEMENT_TEMPLATE.sh explicitly unsets the variable
when it can't derive a positive value, so on the local runner it's absent
and GNU sort accepts --buffer-size=M silently (exit 0, no stderr, no cap).
Emit the flag only when there's a value rather than defaulting to an
invented number, which would be worse than no cap on a big-memory node.
GALAXY_MEMORY_MB rather than the per-slot variant: the two sorts now run
sequentially, one process at a time with the whole job's memory available,
so a per-slot budget combined with --parallel=$GALAXY_SLOTS under-budgets
by a factor of GALAXY_SLOTS.
Also register comp1 1.0.2 -> 1.0.3 in WORKFLOW_SAFE_TOOL_VERSION_UPDATES.
The bump touches only <requirements> and <command>; the inputs are
unchanged, so the update is workflow-safe. Without the entry
_stock_tool_source_for returns None for workflows pinned to 1.0.2.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`format: fastq` is a string, but `YamlDataParameter.format` is a list.
The string form parses only via the `_split_format` before-validator; the
JSON Schema dumped from the model does not declare it, so schema
consumers reject what the prompt teaches. The prompt's own complete
example omits `format` entirely, so the two examples disagreed.
Also state `name`'s `min_length=5`. It was unstated, and short names
("BWA", "STAR") are exactly what a model reaches for.
Neither bites upstream, where `output_type=UserToolSourceAuthoringView`
constrains generation. Both bite consumers using the prompt unconstrained.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
has_class returns a bool rather than asserting, so the fresh-load check in test_basics was silently checking nothing. The bootstrap-vue to GButton swap also renamed the class from disabled to g-disabled, which never got propagated to navigates_galaxy or framework -- both were still asserting on the old name, making them vacuously true. test_history_pages already had this right.
_atomic_download named its temp file after the object it was downloading, so two
downloads of the same uncached object shared one temp path: the second's open()
truncated what the first was still writing, and whichever renamed first
published a corrupt object into the cache to be served as complete.
Give every download its own temp file instead. Concurrent downloads can no
longer interfere, and whichever finishes last publishes a complete copy. Temp
files left behind by a hard kill are reaped by the cache monitor like any other
cache file.
Serving a dataset from a remote object store pulls the whole object into the
local cache before sending a single byte. For large datasets that trips proxy
time-to-first-byte timeouts (504s), and objects bigger than the cache cannot be
downloaded at all because _caching_allowed refuses to pull them.
Tee-streaming proxies the object's bytes straight to the client while writing
them into the cache on the way past, publishing the cache copy atomically once
the whole object has streamed. First byte is immediate, the cache is warmed for
free, and objects too big for the cache stream straight through without a cache
write.
ObjectStore grows get_data_stream(), dispatched like get_direct_download_url();
the tee itself lives in CachingConcreteObjectStore, so a backend only supplies a
raw chunk iterator (boto3, azure and cloud do; every other store keeps today's
pull). A stream that ends short of the object's remote size is rejected rather
than cached, and each concurrent stream writes its own temp file so they cannot
truncate each other's.
Only plain whole-file downloads stream: HEAD and Range requests need random
access, so they still get a cached file, as do previews, chunked display,
extra-files access and archive downloads. This is the internal-backend
complement to the presigned-URL redirect, which only helps object stores whose
URLs a client can reach directly.
Reverses list collection element order during creation and upload to align with the history panel display. Clarifies test comments to document this intentional behavior instead of treating it as an unexpected side effect.
Inline upload auto-selects datasets into the collection builder. Clicking the element would toggle it off and cause test failures. Verifying its presence is now sufficient.
Also renames test to reflect that it is testing inline upload, not modal upload.
Updates the `chunk_upload_size` for test configurations to a more practical 1MB. The previous value of 100 bytes was uncharacteristically small and could lead extremely slow uploads even for small test files generating thousands of chunked requests that could timeout the tests.
The `send_keys` method interprets the tab character `\t` as a keypress that shifts focus away from the input element. This modification ensures that literal tab characters can be inserted into text fields during Selenium interactions by using JavaScript to directly set the element's value when `\t` is present in the input string.
Replaces outdated rule builder interactions with the new wizard component across test files. Improves test reliability by adding waits for loading states and updating selectors to match the current interface.
Update type annotations to use PEP 604 union syntax, replacing older Optional and Union imports. This simplifies the codebase and aligns with modern Python standards while resolving linting issues.
The changes also include stubbing out missing methods in the base staging class to improve type consistency across subclasses.
Adapts existing workflow run tests to interact with the new modal upload interface instead of the embedded panel. Adjusts paste content expectations for newline handling and replaces direct upload calls with the new modal context API. Removes the deprecated `_upload_hello_world_for_input` helper to clean up the test suite.
Extracts common upload staging logic into a `_UploadStaging` base class to share state between panel and modal contexts. Introduces `UploadMethodModalContext` for workflow form inputs and adds an `upload_via_modal` convenience method. Removes verbose docstrings and inline comments throughout the helper classes to improve readability.
Adjusts the Selenium helper for rule-based uploads to match changes in the UI flow. Instead of confirming a selection dialog, the helper now waits for the dialog to close and explicitly advances the file set wizard to reach the rule builder step.
Conflict: packages/tool_util/setup.cfg was deleted on dev by the
pyproject.toml migration (c329529e51) and modified on release_26.1 by
#23256, which moved jsonschema from install_requires to the test extra.
Kept the deletion and ported that move into packages/tool_util/pyproject.toml
so the dependency change isn't lost in the merge forward.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dannon Baker