-
Notifications
You must be signed in to change notification settings - Fork 82
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
Error handling for code with third-party dependencies #315
Comments
Note @jeremyliweishih we should decide if this sort of thing is in-scope for the pipeline project or if we want to punt it |
I think it should be it's own one-off issue and not part of the requirements for the pipeline project as there isn't much tie in with anything else part of the project. We can always add the pipeline project as a blocker for a long term fix and include a simple patch (include all third-party deps so far) first. |
That sounds good to me. In that case I think we should consider this as blocked on the pipeline project, because the solution could change depending on what we change about pipelines |
@kmax12 : My instinct here is that we should complete the phase 1 pipeline project @jeremyliweishih is working on, then circle back to this issue and build a sustainable way to make third-party dependencies optional. And until then, we may continue to merge things like #247 (which add new third-party deps to requirements.txt but also add rudimentary error handling as a fallback), with the understanding we'll circle back and make them optional when we address this issue. Do you have an opinion on that plan? |
@kmax12 said in meeting today:
|
Adding additional notes from comments made in #247 : @dsherry mentioned
I think it could be even worth having at the |
I'll try to knock this out this week. Next step is to list out reqs and desired behavior, and decide on an API. |
@rwedge and I discussed this yesterday. Background on configuring installation via setuptools Note it appears this mechanism supports installing additional packages, and potentially updating the versions of previously installed packages, but not uninstalling packages. That's fine. Installation options
Option 1 feels the most appealing. I'm unaware of any good options for handling this stuff beyond sticking with our current setuptools setup. Code support options
Option 1 feels best to me. Questions for consideration
Proposal
Advantages: Allows users to install evalml without third-party libraries. Long-term, it would be neat if we can find a solution which raises an error or warning earlier than fit-time. @kmax12 do you have an opinion on this issue? |
I did a review of all the dependencies in Packages which are required by evalml and would be quite difficult to make optional:
Packages which could potentially be removed:
Conclusion: we could reduce installer size by a lot if we make more things optional. Does this affect our plan for handling third-party pipelines? We could move towards supporting multiple extras:
This would work fine. It feels overly complicated though. And it raises concerns about testing: we'd at least need an integration test for each to ensure the library works with the subset of deps. An alternative would be to only support two targets, the minimal default and |
Solution discussed with @kmax12 :
An option to consider for long term: build two |
We currently have at least one third-party library dependency (xgboost) and another on the way (catboost #247). This issue tracks figuring out how we present that to users.
Questions:
My current thought is that the code @angela97lin is adding in the catboost PR is a good start, i.e. we have each estimator or component throw an error when the underlying library is missing. But I think we can do more that that too. We could catch that error in the automl, skip the pipeline in question, perhaps print a warning, and continue the search. We could also ask users to install third-party deps by default, by including them in
requirements.txt
, which should cover the majority of cases.Adding related comments made in #247 to keep things in one place:
The text was updated successfully, but these errors were encountered: