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

Add Session Affinity toggle to dr-apps CLI #65

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ben-cutler-datarobot
Copy link
Collaborator

@ben-cutler-datarobot ben-cutler-datarobot commented Dec 17, 2024

This PR does the following:

  1. Refactors the create command to handle runtime params consistently with a update_resources rather than a update_num_replicas or a update_cpu_size
  2. Refactors the create unit test to cover cases where kwargs such as --replicas is unset
  3. Refactors the create unit test to render the multipart data, rather than assume various data blobs are in it.
  4. Adds a --use-session-affinity boolean flag for whether session affinity is set or not in the app.

@engprod-2
Copy link

engprod-2 bot commented Dec 17, 2024

The Needs Review labels were added based on the following file changes.

Team @datarobot/applications (#applications) was assigned because of changes in files:

drapps/create.py
drapps/helpers/custom_app_sources_functions.py
setup.cfg
tests/cli/test_create.py

If you think that there are some issues with ownership, please discuss with C&A domain at #sdtk slack channel and create PR to update DRCODEOWNERS\CODEOWNERS file.

@raulpineda
Copy link
Contributor

This seems like a breaking change to me, unless we keep the update_num_replicas and update_cpu_size and alias them to update_resources I think this needs more than a patch version bump.

@demchukilya thoughts?

Copy link
Collaborator

@demchukilya demchukilya left a comment

Choose a reason for hiding this comment

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

@raulpineda From my perspective this part looks fine.

update_num_replicas and update_cpu_size -- are internal implementations and it's not expected, that user will user those functions directly. User interacts only with CLI commands and from this point only change is added new argument --use-session-affinity.

I am a bit not happy that we are using cpu-size instead of resource bundle, because in most cases not CPU but RAM is biggest limitation factor, but it looks like this is here for long enough.

@engprod-2
Copy link

engprod-2 bot commented Dec 18, 2024

Label Needs Review: Applications was removed because @demchukilya is part of Applications domain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants