Skip to content
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

Feature/simplify CTS usage #230

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

a-solovyev12
Copy link
Member

@a-solovyev12 a-solovyev12 commented May 6, 2021

Description

  • Add a tool for automatic generation of config file for classifier import in CellTypeScan pipelines.
  • Minor changes in other XML tools

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • I have made any required changes to upstream dependencies for a tool wrapper, and they are available in distribution channels (e.g. Pip, Conda).
  • If I have updated the underlying software for a tool wrapper (e.g. scanpy-scripts by changing the value of @TOOL_VERSION@), then I have reset all 'build' values to 0 (e.g. @TOOL_VERSION@+galaxy0)
  • If I have updated a tool wrapper without a software change, then I have bumped the associated 'build' values (e.g. @TOOL_VERSION@+galaxy0 @TOOL_VERSION@+galaxy1)

@pcm32
Copy link
Member

pcm32 commented May 6, 2021

If this is consumed by a single tool later on, I would just add this logic on that tool, otherwise it is an extra box just to write params in YAML.

@pinin4fjords
Copy link
Member

This was my idea, we need to provide config to more than one downstream tool I think.

@pinin4fjords
Copy link
Member

But yes @a-solovyev12 if I've mis-understood and the config is only passed to a single tool, then that tool should have these options instead.

@a-solovyev12
Copy link
Member Author

@pinin4fjords This config is passed to two scripts: a general data import and classifier import ones. Only the latter is relevant in the context of Galaxy, that is why I said it works for a single tool. Putting all of these parameters into that import script will make the options cluttered and basically defeat the purpose of this config file, so in my opinion it's better to leave it as an independent tool.

@pcm32
Copy link
Member

pcm32 commented May 7, 2021

Re options cluttered, you can always hide advanced options. Is it worth it to send a job to the cluster just to create a YAML that will be used once and where there is no flow control involved for the workflow (as in for instance what we do for iterators?)?

@pinin4fjords
Copy link
Member

I agree, if the options only pertain to one (or even two) tools then they belong on that tool rather than a separate config step. This is partly down to my confusion, so apologies for that.

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.

3 participants