OpenMS place each in/output symlink in unique dir #555
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bernt-matthias
changed the title
bernt-matthias
force-pushed
the
topic/openms-unique-in-out
branch
2 times, most recently
from
31e113d to
b4071d7
Compare
bernt-matthias
force-pushed
the
topic/openms-unique-in-out
branch
from
8dd7f29 to
8c188a0
Compare
bgruening
reviewed
Feb 10, 2021
bernt-matthias
force-pushed
the
topic/openms-unique-in-out
branch
from
2fb7ed3 to
06ae229
Compare
bernt-matthias
force-pushed
the
topic/openms-unique-in-out
branch
from
07c5907 to
f13592b
Compare
bgruening
reviewed
Feb 18, 2021
| @@ -24,7 +24,8 @@ mkdir out && | |||
| #if $adv_opts_cond.adv_opts_selector=='advanced': | |||
| #if $adv_opts_cond.reannotate_filenames: | |||
| mkdir adv_opts_cond.reannotate_filenames && | |||
| ${ ' '.join(["ln -s '%s' 'adv_opts_cond.reannotate_filenames/%s.%s' &&" % (_, re.sub('[^\w\-_]', '_', _.element_identifier), $gxy2omsext(_.ext)) for _ in $adv_opts_cond.reannotate_filenames if _]) } | |||
| mkdir ${' '.join(["'adv_opts_cond.reannotate_filenames/%s'" % (i) for i, f in enumerate($adv_opts_cond.reannotate_filenames) if f])} && | |||
| ${' '.join(["ln -s '%s' 'adv_opts_cond.reannotate_filenames/%s/%s.%s' && " % (f, i, re.sub('[^\w\-_]', '_', f.element_identifier), $gxy2omsext(f.ext)) for i, f in enumerate($adv_opts_cond.reannotate_filenames) if f])} | |||
There was a problem hiding this comment.
@bernt-matthias a very general question, should we maybe have f.save_element_identifier that is doing this inside Galaxy, s that we can avoid all this sed/replace stuff in the wrappers?
There was a problem hiding this comment.
Yeah! This is used in so many tools. Started almost 2 years ago with this galaxyproject/galaxy#7937 .. somehow got distracted. We should try to get this in 21.05.
There was a problem hiding this comment.
Oh gosh, complete forget about it ... 💯
bernt-matthias
force-pushed
the
topic/openms-unique-in-out
branch
2 times, most recently
from
9853199 to
6a75bcb
Compare
|
Hi @bgruening , do you think we can merge soon (and if you like selectively push at least ConsensusID to the TS)? The latest test fails for the online mascot online adapter (maybe we should set this on the skip list .. which is not possible yet galaxyproject/planemo-ci-action#13). I guess this is transient, but I could ask at the OpenMS gitter channel. For an overview let me summarize the changes (The first can be critical - I need it in particular for ConsensusID. The last two are cosmetic):
The complete list of 35 affected tools
OpenMS sets text, int, select, bool parameters that have a default as optional (required=False), the default value is set implicitly if no value is given. This is reasonable for the CLI because one certainly does not want the user to specify the default manually for all parameters. For Galaxy tools setting these parameters as required leads to the equivalent behavior. Assuming required is better because it makes the implicit setting of parameters more transparent to the user (in Galaxy the default would be prefilled in the form and at least one option needs to be selected).
|
bgruening
approved these changes
Feb 21, 2021
|
That looks all pretty great! I would prefer if you only push ConsensusID to the TS if that is possible and wait for the other enhancements for a new OpenMS release. If possible. |
|
Thanks a lot!!! |
before each symlink was placed in a dir named by the name of the corresponding parameter. this still allows for conflicts if several data sets have the sem identifier. solution: place symlinks in directory parameter_name/galaxy-dataset-id usecase: ConsensusID takes the result of different search engines for the same sample. then the identifiers of the inputs are all the sample name which leads to a conflict.
for outputs with a type selector set the default format and only have change_format for the others
nicer (less potential info leakage to the user) and simpler. also no need to import os in the command section
OpenMS uses sets text, int, select, bool parameters that have a default as optional (required=False), the default value is set implicitly if no value is given. This is reasonable for the CLI because one certainly does not want the user to specify the default manually for all parameters. For Galaxy tools setting these parameters as required leads to the equivalent behavior. Assuming required is better because it makes the implicit setting of parameters more transparent to the user (in Galaxy the default would be prefilled in the form and at least one option needs to be selected).
adapt test generation (get default from CTD file if missing in INI files)
bernt-matthias
force-pushed
the
topic/openms-unique-in-out
branch
from
6a75bcb to
7585813
Compare
Will do. |
|
I cancelled the CI workflow and uploaded ConsensusID manuallyto the TS. |
bernt-matthias
added a commit
to bernt-matthias/tools-galaxyp
that referenced
this pull request
Mar 17, 2022
with galaxyproteomics#555 galaxyproteomics@fecd2cc galaxyproteomics@ffdd5c5 optional (non file) parameters were rendered as required in the galaxy xml this commit tries to revert this, since it breaks the tests for Comet Adapter, where the modification select parameters also allow to select no parameter (and in the tests this is really done)
bernt-matthias
added a commit
to bernt-matthias/tools-galaxyp
that referenced
this pull request
Mar 18, 2022
with galaxyproteomics#555 galaxyproteomics@fecd2cc galaxyproteomics@ffdd5c5 optional (non file) parameters were rendered as required in the galaxy xml this commit tries to revert this, since it breaks the tests for Comet Adapter, where the modification select parameters also allow to select no parameter (and in the tests this is really done)
bernt-matthias
added a commit
to bernt-matthias/tools-galaxyp
that referenced
this pull request
Mar 19, 2022
with galaxyproteomics#555 galaxyproteomics@fecd2cc galaxyproteomics@ffdd5c5 optional (non file) parameters were rendered as required in the galaxy xml this commit tries to revert this, since it breaks the tests for Comet Adapter, where the modification select parameters also allow to select no parameter (and in the tests this is really done)
bernt-matthias
added a commit
to bernt-matthias/tools-galaxyp
that referenced
this pull request
Apr 5, 2022
with galaxyproteomics#555 galaxyproteomics@fecd2cc galaxyproteomics@ffdd5c5 optional (non file) parameters were rendered as required in the galaxy xml this commit tries to revert this, since it breaks the tests for Comet Adapter, where the modification select parameters also allow to select no parameter (and in the tests this is really done)
bernt-matthias
added a commit
to bernt-matthias/tools-galaxyp
that referenced
this pull request
Oct 4, 2022
with galaxyproteomics#555 galaxyproteomics@fecd2cc galaxyproteomics@ffdd5c5 optional (non file) parameters were rendered as required in the galaxy xml this commit tries to revert this, since it breaks the tests for Comet Adapter, where the modification select parameters also allow to select no parameter (and in the tests this is really done)
bernt-matthias
added a commit
to bernt-matthias/tools-galaxyp
that referenced
this pull request
Nov 8, 2022
with galaxyproteomics#555 galaxyproteomics@fecd2cc galaxyproteomics@ffdd5c5 optional (non file) parameters were rendered as required in the galaxy xml this commit tries to revert this, since it breaks the tests for Comet Adapter, where the modification select parameters also allow to select no parameter (and in the tests this is really done)
bernt-matthias
added a commit
to bernt-matthias/tools-galaxyp
that referenced
this pull request
Nov 24, 2022
with galaxyproteomics#555 galaxyproteomics@fecd2cc galaxyproteomics@ffdd5c5 optional (non file) parameters were rendered as required in the galaxy xml this commit tries to revert this, since it breaks the tests for Comet Adapter, where the modification select parameters also allow to select no parameter (and in the tests this is really done)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
before each symlink was placed in a dir named by the name of the corresponding parameter. this still allows for conflicts if several
data sets have the same identifier.
solution: place symlinks in directory parameter_name/galaxy-dataset-id
usecase: ConsensusID takes the result of different search engines for the same sample. then the identifiers of the inputs are all the sample name which leads to a conflict.
question is how we can manage the tool version bump since we manually bumped some of the tools (bump.json). we could continue to bump manually, or just use the macro token and increase it to max bump + 1.
For most tools the change is just an additional import.. I could also change the converter such that the import is only added if needed.