-
Notifications
You must be signed in to change notification settings - Fork 446
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
Allow multi step configuration for skypilot #2166
Changes from 9 commits
27fc323
6f50333
f109795
11a18b9
db2c007
4471d48
57ed6df
0e02dee
7452667
32de4aa
0eb1f97
4270edd
dfdedf9
b6c9f4c
301bfa6
f9d51d8
559b710
7476f85
09c368d
4077eda
5057bae
b20b975
8699b5a
9898aef
ec7d09b
21af350
69b5924
9964220
f65e61d
b33d4bb
daaaefc
dddd6fe
a3271a6
794cba4
f40796f
46170b6
89903b8
44ea432
df16014
69a7ce4
7ef6179
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
import base64 | ||
import datetime | ||
import json | ||
import os | ||
import re | ||
from typing import Any, Dict, List, Optional, Tuple, cast | ||
|
||
|
@@ -1371,6 +1372,21 @@ def _configure_local_client( | |
"aws_session_token" | ||
] = credentials.token | ||
|
||
aws_credentials_path = os.path.join( | ||
users_home, ".aws", "credentials" | ||
) | ||
|
||
# Ensure the .aws directory exists | ||
os.makedirs(os.path.dirname(aws_credentials_path), exist_ok=True) | ||
|
||
# Ensure the credentials file exists | ||
if not os.path.isfile(aws_credentials_path): | ||
open(aws_credentials_path, "a").close() | ||
safoinme marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# Set the appropriate permissions for the .aws directory and credentials file | ||
os.chmod(os.path.dirname(aws_credentials_path), 0o700) | ||
os.chmod(aws_credentials_path, 0o600) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure this works on every OS? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @safoinme FYI, this is the piece of code in the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @safoinme can you use the code I suggested here ? |
||
|
||
common.rewrite_credentials_file(all_profiles, users_home) | ||
|
||
logger.info( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldnt we take this from
ResourceSettings
actually now that I think about it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the GPU
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to distinguish between both of them, because
ResourceSettings
can be defined for things like pods which would allow random resource given, while for Skypilot most of these parameters must match if supplied together because a VM type with this specific resource must existThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the
cpu_count
andmemory
in ResourceSettings should be mapped here... what is the point of ResourceSettings otherwise? @schustmi would love your feedback