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 --memory option to paasta local-run to allow overriding of memory… #3823

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EvanKrall
Copy link
Member

@EvanKrall EvanKrall commented Apr 3, 2024

… limit set in yelpsoa-configs.

In my first attempt at this branch, I tried adding a memory argument to run_docker_container, and having

-    memory = instance_config.get_mem()
+    if memory is None:
+        memory = instance_config.get_mem()

However, the actual docker command has --mem-swap={instance_config.get_mem() + 64} (via format_docker_parameters calling get_mem_swap), and also we sometimes have environment variables that reference instance_config.get_mem(), so just modifying instance_config.config_dict["mem"] seems like a more complete solution.

jfongatyelp
jfongatyelp previously approved these changes Apr 3, 2024
Copy link
Member

@nemacysts nemacysts left a comment

Choose a reason for hiding this comment

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

would we also want to add similar versions of this for the other resource options?

(i also think that we didn't add this initially since we were expecting folks to use the -y flag to a local soaconfigs with the edited values)

@EvanKrall
Copy link
Member Author

EvanKrall commented Apr 4, 2024

would we also want to add similar versions of this for the other resource options?

Ah, at first I thought we weren't even setting CPU limits because I only saw instance_config.get_cpus() referenced once in local_run.py, but it looks likeformat_docker_parameters also sets --cpu-quota based on cpus. I'll add --cpus and --disk.

(i also think that we didn't add this initially since we were expecting folks to use the -y flag to a local soaconfigs with the edited values)

yeah maybe, but adhoc batch runs are enough of a use case and it's a little unfortunate to tell people to go edit yelpsoa-configs (or make a local checkout and run with -y) just to rerun their batch so it doesn't OOM.

…run to allow overriding of settings from yelpsoa-configs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants