-
Notifications
You must be signed in to change notification settings - Fork 121
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
chore: clean deployment #677
Conversation
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.
Great! I find that removing dependencies is very satisfying :)
@@ -23,7 +23,7 @@ | |||
|
|||
import boto3 | |||
|
|||
from ..deployment.utils import filter_logs, wait_for_connection_to_be_available | |||
from .utils_server import filter_logs, wait_for_connection_to_be_available |
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.
are you missing some files here? in the utils_server
dir?
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.
utils_server is a py file. Should be good.
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.
don't you need to add the dependencies removed from pyproject.toml here ?
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.
Yes certainly!
@@ -325,7 +325,6 @@ def dump_dict(self) -> Dict: | |||
# Scikit-Learn | |||
metadata["alpha"] = self.alpha | |||
metadata["fit_intercept"] = self.fit_intercept | |||
metadata["solver"] = self.solver |
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.
any idea why these are removed ?
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 comes from the new test we added recently
concrete-ml/tests/sklearn/test_sklearn_models.py
Line 2057 in 6921c91
def test_initialization_variables_and_defaults_match( |
It highlighted a few discrepancies with sklearn models. Here solver
isn't actually part of this model in sklearn.
@@ -1291,7 +1292,8 @@ class BaseTreeEstimatorMixin(BaseEstimator, sklearn.base.BaseEstimator, ABC): | |||
#: Model's base framework used, either 'xgboost' or 'sklearn'. Is set for each subclasses. | |||
framework: str | |||
|
|||
def __init_subclass__(cls): | |||
def __init_subclass__(cls, *args, **kwargs): | |||
super().__init_subclass__(*args, **kwargs) |
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.
can you explain these changes please ?
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.
Still related to the new test.
@@ -32,19 +32,3 @@ The server-side implementation of a Concrete ML model follows the diagram above. | |||
## Example notebook | |||
|
|||
For a complete example, see [the client-server notebook](../advanced_examples/ClientServer.ipynb) or [the use-case examples](../../use_case_examples/deployment/). | |||
|
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.
maybe move these lines to the deployment
directory in use_case_examples
?
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.
Some comments from @andrei-stoian-zama but otherwise fine with me
a607a5b
to
239192a
Compare
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.
your new commit seems to delete all the deployment use case
684d35c
to
517672a
Compare
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.
Looks good! How can we test the cifar/sentiment/breast cancer deployment use cases using the CI ?
b1c16af
to
33f441d
Compare
0710344
to
68157c5
Compare
68157c5
to
277a3f2
Compare
Coverage passed ✅Coverage details
|
closes https://github.com/zama-ai/concrete-ml-internal/issues/4411
closes https://github.com/zama-ai/concrete-ml-internal/issues/4388
closes https://github.com/zama-ai/concrete-ml-internal/issues/3927
closes https://github.com/zama-ai/concrete-ml-internal/issues/3561
closes https://github.com/zama-ai/concrete-ml-internal/issues/3598
closes https://github.com/zama-ai/concrete-ml-internal/issues/4064
closes https://github.com/zama-ai/concrete-ml-internal/issues/4193