Skip to content

GSK-2896 Automatically generate a Kernel based on dependencies#1829

Merged
Hartorn merged 6 commits intomulti-ml-workerfrom
GSK-2896
Mar 7, 2024
Merged

GSK-2896 Automatically generate a Kernel based on dependencies#1829
Hartorn merged 6 commits intomulti-ml-workerfrom
GSK-2896

Conversation

@kevinmessiaen
Copy link
Copy Markdown
Member

Description

Automatically generate a Kernel based on dependencies.
If any kernel match current dependencies, then use it instead

Related Issue

Type of Change

  • 📚 Examples / docs / tutorials / dependencies update
  • 🔧 Bug fix (non-breaking change which fixes an issue)
  • 🥂 Improvement (non-breaking change which improves an existing feature)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 🔐 Security fix

@linear
Copy link
Copy Markdown

linear Bot commented Mar 4, 2024

@kevinmessiaen kevinmessiaen marked this pull request as ready for review March 4, 2024 05:45
Comment thread giskard/client/giskard_client.py Outdated
Comment thread giskard/client/giskard_client.py
if len(matching_kernels) > 0:
return matching_kernels[0]["name"]

self._session.post(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we check the result here? Cuz for "PROCESS" kernel, backend will create an env as well. This can fail sometimes

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this object is doing it automatically, contrary to request ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we should change backend, to make start install deps and freeze if needed ?
We could make it so it save in DB the kernel, and then immediatly launch an async task in job manager to do install + freeze (+start ?) if needed ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(unrelated to this PR)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this object is doing it automatically, contrary to request ?

Yes, you are right. So it will raise the exception in case of failed creation, right?

Comment thread giskard/client/giskard_client.py Outdated
def initialize_kernel(self):
python_version = f"{sys.version_info[0]}.{sys.version_info[1]}"
base_kernel_name = f"{gethostname()}-{python_version}"
kernel_name = f"{base_kernel_name}-{uuid.uuid4()}"
Copy link
Copy Markdown
Member

@Hartorn Hartorn Mar 4, 2024

Choose a reason for hiding this comment

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

I think we could go with something simpler, like <project_key>_<auto>

Copy link
Copy Markdown
Member

@Hartorn Hartorn left a comment

Choose a reason for hiding this comment

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

Main issue is with project creation error, and kernel is still created

},
)

if kernel_name is None:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Only issue here for me is if you try to create a project already existing, you will re-create a kernel everytime.
That's why changing kernel_name to <project_key>_<auto> (or at least without random/uuid in name) will protect that, since kernel creation is checking that

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I updated the kernel creation to use the project_key. It should not create a new kernel if the project key already exists (granted it has same dependencies)

if len(matching_kernels) > 0:
return matching_kernels[0]["name"]

self._session.post(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this object is doing it automatically, contrary to request ?

Copy link
Copy Markdown
Member

@Inokinoki Inokinoki left a comment

Choose a reason for hiding this comment

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

LGTM, let's just handle @Hartorn 's comments on the case where project fails to create

# A different kernel exists that uses the same name
kernel_name = f"{kernel_name}_{uuid.uuid4()}"

self._session.post(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we extract this into a method like create_kernel in Giskard client?

I am also trying to do something similar for list kernels. Otherwise, I can do it on my side after merging this branch.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure it's done

@kevinmessiaen
Copy link
Copy Markdown
Member Author

LGTM, let's just handle @Hartorn 's comments on the case where project fails to create

It's done, when the key already exists it'll reuse kernel of same key and won't create a new one.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Mar 7, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
30.0% Coverage on New Code (required ≥ 70%)

See analysis details on SonarCloud

@Hartorn
Copy link
Copy Markdown
Member

Hartorn commented Mar 7, 2024

Looks good! Just need to test it on my side

@Hartorn Hartorn dismissed Inokinoki’s stale review March 7, 2024 11:43

I think it's handled

@Hartorn Hartorn merged commit 71e5369 into multi-ml-worker Mar 7, 2024
@Hartorn Hartorn deleted the GSK-2896 branch March 7, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants