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

Remove heavy dependencies #163

Open
ebolyen opened this issue Sep 28, 2023 · 7 comments
Open

Remove heavy dependencies #163

ebolyen opened this issue Sep 28, 2023 · 7 comments

Comments

@ebolyen
Copy link
Collaborator

ebolyen commented Sep 28, 2023

It looks like we may be able to drop some rather significant weight from the recipe:

    - q2-longitudinal {{ qiime2_epoch }}.* 
    - q2-feature-classifier {{ qiime2_epoch }}.*

I can't find an import of longitudinal, which is bringing along sample-classifier as well.
And for feature-classifier, the only imports are these which I don't think is actually worth the cost of pulling feature-classifier along (requiring quality-control as well).

@nbokulich
Copy link
Collaborator

Yeah the longitudinal dependency can definitely be dropped... I think that is historical.

dropping q2-feature-classifier would require dropping a couple actions in RESCRIPt — is that what you are proposing? Or add instructions that users who wish to run those actions will need to install q2-feature-classifier separately?

@ebolyen
Copy link
Collaborator Author

ebolyen commented Sep 29, 2023

No, I would want to keep those actions, but I think we can just duplicate the parameter descriptions (and move the type over to q2-types) (from these imports here:

from q2_feature_classifier.classifier import (_parameter_descriptions,
_classify_parameters)
from q2_feature_classifier._taxonomic_classifier import TaxonomicClassifier
).

Is there a pipeline that uses feature-classifier directly that I am missing?

@ebolyen ebolyen added this to 2024.2 Sep 29, 2023
@ebolyen ebolyen moved this to Backlog in 2024.2 Sep 29, 2023
@nbokulich
Copy link
Collaborator

Is there a pipeline that uses feature-classifier directly that I am missing?

Yes, see e.g.:

fit = ctx.get_action('feature_classifier', 'fit_classifier_naive_bayes')
classify = ctx.get_action('feature_classifier', 'classify_sklearn')

I would be very tempted to deprecated these actions though. Maybe just port those pipelines over into the classifier training workflows to use them for our purposes. I was never very happy with those pipelines.

@lizgehret
Copy link
Collaborator

This seems like a good candidate to finish up in this upcoming release cycle since we are adding RESCRIPt to the amplicon distro. @nbokulich I can bring this up in this week's eng meeting - your preference would still be to deprecate these actions?

@nbokulich
Copy link
Collaborator

Hi @lizgehret thanks for the reminder! Yes let's discuss this week.

Which dependencies are priority to drop? It looks like q2-longitudinal is indeed used in a couple places, e.g., here:

volatility = ctx.get_action('longitudinal', 'volatility')

though it is just to get the line plots from the volatility plot. So I would favor waiting until we have similar functionality up and running in q2-visard before dropping this.

The actions depending on q2-feature-classifier could always be ported over to another plugin (a new one) to simplify RESCRIPt's dependencies and focus more on the core functionality. But as q2-feature-classifier is already in the amplicon distro anyway, is this needed?

@lizgehret lizgehret moved this from Backlog to In Analysis in 2024.2 Feb 14, 2024
@nbokulich
Copy link
Collaborator

q2-longitudinal is dropped as a dependency with #204

so we are halfway there. Should we discuss if/how to address the q2-feature-classifier dependency? This will require dropping or porting a couple actions. Porting to a new plugin could be an option, I think the classifier eval actions are probably not needed by most users.

@lizgehret
Copy link
Collaborator

q2-longitudinal is dropped as a dependency with #204

so we are halfway there. Should we discuss if/how to address the q2-feature-classifier dependency? This will require dropping or porting a couple actions. Porting to a new plugin could be an option, I think the classifier eval actions are probably not needed by most users.

Yeah! Maybe this would be a good thing to discuss in next month's meeting post-release? Once we have all of the grant objectives wrapped up, it'll be a good time to figure out what we can add to our plates for the next development cycle.

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

No branches or pull requests

3 participants