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.

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

Switching from mlWorkerId to kernel name #1777

Merged

Conversation

Hartorn
Copy link
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 self-assigned this Feb 1, 2024
Copy link

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 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
Copy link

sonarcloud bot commented Feb 14, 2024

@Hartorn Hartorn marked this pull request as ready for review February 19, 2024 08:24
Copy link
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

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

"""
# TODO(Bazire): handle properly the "auto" detection/creation of kernel
Copy link
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
15 of 16 checks passed
@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.

None yet

3 participants