in tool schema. New lxml is more strict when validating
the xml schema and fails with
```
lxml.etree.XMLSchemaParseError: complex type 'Output': The content model is not determinist., line 5329
```
this is because `filter` and `discover_datasets` are present in
OutputDataElement and OutputCollectionElement, making
```
<xs:sequence>
<xs:group ref="OutputDataElement" minOccurs="0" maxOccurs="unbounded" />
<xs:group ref="OutputCollectionElement" minOccurs="0" maxOccurs="unbounded" />
</xs:sequence>
```
not deterministic.
In any case this isn't an accurate model of what is allowed and parsed,
as you can't use collection-specific discover_datasets options outside
of a dataset collection.
I **think** that the reason for adding
OutputCollectionElement to the sequence is that you can have a `data`
element nested in a `collection` element.
To continue allowing this and making it more precise I've added an
additional `OutputCollectionDataElement` type that is allowed within
`collection`. This then should allow us to remove
`OutputCollectionElement` from the `OutputData` type.
A quick test against IUC and devteam revealed no problem with this
approach per se, however it showed that https://github.com/galaxyproject/tools-iuc/blob/aa8360cb3ec9faf1488938a430855977632706ff/tools/krakentools/extract_kraken_reads.xml#L145
uses `change_format` which is not implemented for collections.
This will always fall back to a string, but the intention of the code reads better and we may add some custom validation later to really check for the URL correctness.
Co-authored-by: Marius van den Beek <m.vandenbeek@gmail.com>
I think the premise that we want to do a rollback on exceptions in this
method is wrong (it **may** be correct apprach in other places in the
codebase e.g. in
`Tool.handle_single_execution()`). Here it prevents us from comitting
anything inside the with statement (as the job_wrapper.fail method
does).
Here's the simplified issue:
```shell
❯ python -i scripts/db_shell.py -c config/galaxy.yml
>>> with sa_session() as session, session.begin():
... sa_session.execute(update(Job).where(Job.id == 1).values(state="error"))
... sa_session.commit()
... sa_session.execute(update(Job).where(Job.id == 1).values(state="ok"))
... sa_session.commit()
...
<sqlalchemy.engine.cursor.LegacyCursorResult object at 0x11f1be350>
Traceback (most recent call last):
File "<stdin>", line 4, in <module>
File "<string>", line 2, in execute
File "/Users/mvandenb/src/galaxy/.venv/lib/python3.11/site-packages/sqlalchemy/orm/session.py", line 1711, in execute
conn = self._connection_for_bind(bind, close_with_result=True)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/mvandenb/src/galaxy/.venv/lib/python3.11/site-packages/sqlalchemy/orm/session.py", line 1552, in _connection_for_bind
TransactionalContext._trans_ctx_check(self)
File "/Users/mvandenb/src/galaxy/.venv/lib/python3.11/site-packages/sqlalchemy/engine/util.py", line 199, in _trans_ctx_check
raise exc.InvalidRequestError(
sqlalchemy.exc.InvalidRequestError: Can't operate on closed transaction inside context manager. Please complete the context manager before emitting further commands.
```
It is probably still worthwhile to have the job recovery be minimal and
do things such as calling the job wrapper fail method that does actual
work to the job handler as in
https://github.com/galaxyproject/galaxy/pull/17083/, but that's
refactoring that can be done on the dev branch and it still seems risky
in the sense that we then need to be very careful in ensuring we don't
commit anywhere else inside the scope of the begin() statement.
Finally I don't think it makes sense that the startup check should
ever cause the boot process to fail. This isn't a misconfiguration
or even anything catastrophic for the remaining jobs and places
unnecessary stress on admins and can basically break at any time
and shouldn't cause a complete service failure.
Fixes https://github.com/galaxyproject/galaxy/issues/17079
John Davis