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.

Sign up for GitHub

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

Jump to bottom

chore: clean deployment #677

Merged
merged 6 commits into from
May 21, 2024
Merged

chore: clean deployment #677

merged 6 commits into from
May 21, 2024

Conversation

jfrery
Copy link
Collaborator

@jfrery jfrery commented May 9, 2024 •
edited

@jfrery jfrery requested a review from a team as a code owner May 9, 2024 00:51
@cla-bot cla-bot bot added the cla-signed label May 9, 2024
andrei-stoian-zama
andrei-stoian-zama requested changes May 16, 2024
View reviewed changes
Copy link
Collaborator

@andrei-stoian-zama andrei-stoian-zama left a 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 :)

use_case_examples/deployment/server/deploy_to_aws.py Outdated
@@ -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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

use_case_examples/deployment/server/server_requirements.txt Outdated
Copy link
Collaborator

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 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes certainly!

src/concrete/ml/sklearn/glm.py
@@ -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
Copy link
Collaborator

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 ?

Copy link
Collaborator Author

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.

src/concrete/ml/sklearn/base.py
@@ -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)
Copy link
Collaborator

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 ?

Copy link
Collaborator Author

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.

docs/guides/client_server.md
@@ -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/).

Copy link
Collaborator

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 ?

fd0r
fd0r previously approved these changes May 16, 2024
View reviewed changes
Copy link
Collaborator

@fd0r fd0r left a 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

andrei-stoian-zama
andrei-stoian-zama requested changes May 16, 2024
View reviewed changes
Copy link
Collaborator

@andrei-stoian-zama andrei-stoian-zama left a 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

andrei-stoian-zama
andrei-stoian-zama previously approved these changes May 17, 2024
View reviewed changes
Copy link
Collaborator

@andrei-stoian-zama andrei-stoian-zama left a 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 ?

@jfrery jfrery force-pushed the chore/clean_deployment branch 4 times, most recently from 0710344 to 68157c5 Compare May 17, 2024 11:10
@github-actions GitHub Actions
Copy link

Coverage passed ✅

Coverage details 

---------- coverage: platform linux, python 3.8.18-final-0 -----------
Name    Stmts   Miss  Cover   Missing
-------------------------------------
TOTAL    7584      0   100%

59 files skipped due to complete coverage.

@jfrery jfrery merged commit 964f463 into main May 21, 2024
16 of 17 checks passed
@jfrery jfrery deleted the chore/clean_deployment branch May 21, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrei-stoian-zama andrei-stoian-zama andrei-stoian-zama approved these changes

@fd0r fd0r Awaiting requested review from fd0r

Assignees
No one assigned
Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jfrery @fd0r @andrei-stoian-zama