remove field _is_sky_managed for intermediate bucket #4545
Conversation
| def _is_sky_managed_intermediate_bucket(bucket_name: str) -> bool: | ||
| return re.match(r'skypilot-filemounts-.+-[a-f0-9]{8}', | ||
| bucket_name) is not None |
There was a problem hiding this comment.
Basing on bucket name is not a robust solution. Can't we manually set the force_delete accordingly in maybe_translate...?
There was a problem hiding this comment.
_force_delete is already True in our cases. The controller will call delete anyway; it just doesn’t know whether the bucket is sky_managed. Should it delete the entire bucket or just the sub_path
| _is_sky_managed_intermediate_bucket( | ||
| self.name)) |
There was a problem hiding this comment.
is_sky_managed should not be inferred on bucket name, it depends on whether the bucket is created by sky or not. Can we instead pass this information to the controller using force_delete, since this field is used mainly for deletion logic?
There was a problem hiding this comment.
_force_delete is already True in our cases. The controller will call delete anyway; it just doesn’t know whether the bucket is sky_managed. Should it delete the entire bucket or just the sub_path
See comment
Test plan
smoke test
custom test
Without custom intermediate bucket config
bucket deleted
With custom intermediate bucket config
bucket persist, sub path deleted
(sky) ➜ cat ~/.sky/config.yaml jobs: bucket: s3://bucket-admin-test/Tested (run the relevant ones):
bash format.shpytest tests/test_smoke.pypytest tests/test_smoke.py::test_fill_in_the_nameconda deactivate; bash -i tests/backward_compatibility_tests.sh