Skip to content

Switching from mlWorkerId to kernel name#1777

Merged
Hartorn merged 10 commits intomulti-ml-workerfrom
feature/gsk-2722-making-the-kernel-actually-start-worker
Feb 26, 2024
Merged

Switching from mlWorkerId to kernel name#1777
Hartorn merged 10 commits intomulti-ml-workerfrom
feature/gsk-2722-making-the-kernel-actually-start-worker

Conversation

@Hartorn
Copy link
Copy Markdown
Member

@Hartorn Hartorn commented Feb 1, 2024

Description

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

Checklist

  • I've read the CODE_OF_CONDUCT.md document.
  • I've read the CONTRIBUTING.md guide.
  • I've updated the code style using make codestyle.
  • I've written tests for all new methods and classes that I created.
  • I've written the docstring in Google format for all the methods and classes that I used.

@Hartorn Hartorn requested a review from andreybavt February 1, 2024 09:49
@Hartorn Hartorn self-assigned this Feb 1, 2024
@linear
Copy link
Copy Markdown

linear Bot commented Feb 1, 2024

@Hartorn Hartorn added the Lockfile Temporary label to update pdm.lock label Feb 1, 2024
@github-actions github-actions Bot removed the Lockfile Temporary label to update pdm.lock label Feb 1, 2024
@Hartorn Hartorn added the Lockfile Temporary label to update pdm.lock label Feb 1, 2024
@github-actions github-actions Bot removed the Lockfile Temporary label to update pdm.lock label Feb 1, 2024
@Hartorn Hartorn added Docker Trigger Docker build for PR and removed Docker Trigger Docker build for PR labels Feb 5, 2024
Hartorn and others added 2 commits February 6, 2024 10:43
@Hartorn Hartorn added the Lockfile Temporary label to update pdm.lock label Feb 14, 2024
@github-actions github-actions Bot removed the Lockfile Temporary label to update pdm.lock label Feb 14, 2024
@sonarqubecloud
Copy link
Copy Markdown

@Hartorn Hartorn marked this pull request as ready for review February 19, 2024 08:24
@kevinmessiaen kevinmessiaen self-requested a review February 19, 2024 09:54
Copy link
Copy Markdown
Member

@kevinmessiaen kevinmessiaen left a comment

Choose a reason for hiding this comment

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

Not sure if I'm doing something wrong but I got this error:

GiskardError: Not Found: Python kernel not found by key: No Kernel found with name external_worker

I ran this piece of code:

from giskard import GiskardClient

url = "http://localhost:9000"
api_key = "gsk-..."

# Create a giskard client to communicate with Giskard
client = GiskardClient(url, api_key)

client.create_project('test', 'test', 'external_worker', 'Empty desc')

and the worker have external_worker as kernel name:

image

The project created in giskard

"""
# TODO(Bazire): handle properly the "auto" detection/creation of kernel
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 really think that we need an auto detection and creation of kernel for ease of use.

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.

Do you think that we can create/use the kernel with the project id as name?

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.

Yes either by a project id or some name generated like: {os_host}-{python_version}

If we find another kernel using the same name with different dependencies we just do {os_host}-{python_version}-{random-value}

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.

@Inokinoki Inokinoki self-requested a review February 21, 2024 16:21
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 in general

The project created in giskard

"""
# TODO(Bazire): handle properly the "auto" detection/creation of kernel
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.

Do you think that we can create/use the kernel with the project id as name?

@Hartorn Hartorn merged commit 8403f75 into multi-ml-worker Feb 26, 2024
@Hartorn Hartorn deleted the feature/gsk-2722-making-the-kernel-actually-start-worker branch February 26, 2024 09:13
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