-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check prefix before complete loading when checking yaml and cwl tools #12604
Check prefix before complete loading when checking yaml and cwl tools #12604
Conversation
Is this coming through |
@@ -21,7 +21,7 @@ | |||
TOOL_REGEX = re.compile(r"<tool\s") | |||
DATA_MANAGER_REGEX = re.compile(r"\stool_type=\"manage_data\"") | |||
|
|||
YAML_EXTENSIONS = [".yaml", ".yml", ".json"] | |||
YAML_EXTENSIONS = [".yaml", ".yml"] | |||
CWL_EXTENSIONS = YAML_EXTENSIONS + [".cwl"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does adding .json to CWL_EXTENSIONS in the next line to make up for this help anything or does that revert the performance. I would be fine with that change but I'm not sure it helps. I've seen CWL json files I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not have any effect on the performance since: looks_like_a_tool also rund looks_like_a_tool_cwl
(because it is in BETA_TOOL_CHECKERS
) which considers the CWL_EXTENSIONS
:
for tool_checker in BETA_TOOL_CHECKERS.values(): |
No, this function seems not to be called at all during The trace is:
|
From the committers meeting: This should be implemented in a different way: e.g. by just checking header lines of potential tools. |
As a side node the tool parser factory only considers
|
Also this logger output is pretty confusing:
So, we are sure that YAML tools will never work? |
cdf9253
to
4a257e8
Compare
Implemented a prefix check for yaml and cwl tools. For the IUC repo
Also checked that the output is equal. |
3357036
to
86d7467
Compare
Co-authored-by: Marius van den Beek <m.vandenbeek@gmail.com>
I was wondering why
planemo ci_find_repos tools-iuc/
(andci_find_tools
) need so much time: approx (3min). Turns out that alljson
files (some of them seem to be quite big .. which is a problem on its own) in there are parsed and checked if they are tools. Essentially almost the complete run time is caused by json files intools/hamronization
andtools/hyphy
.With this change the runtime is approx 10sec.
Support for json has been added here 0da66ee (before this
json
was explicitly forbidden) and likely is desired:I'm pretty sure that the PR is unacceptable as-is .. but should serve as a starting point for discussion:
How to test the changes?
(Select all options that apply)
License