-
Notifications
You must be signed in to change notification settings - Fork 26
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
Comments
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? |
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: RESCRIPt/rescript/plugin_setup.py Lines 40 to 42 in 11aedaa
Is there a pipeline that uses feature-classifier directly that I am missing? |
Yes, see e.g.: RESCRIPt/rescript/cross_validate.py Lines 38 to 39 in 11aedaa
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. |
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? |
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: Line 51 in a19d106
though it is just to get the line plots from the 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? |
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. |
It looks like we may be able to drop some rather significant weight from the recipe:
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).
The text was updated successfully, but these errors were encountered: