Integration of IBM's Cloud Object Storage: COS #1966

Cohen-J-Omer · 2023-05-18T10:55:09Z

Support for IBM's COS.

Notes:

As requested here, I've updated documentation(i.e. getting-started/installation.rst) to provide instructions for setting up IBM credentials.
As this issue remains unresolved, sky check still returns IBM:enabled, even if the necessary fields for storage are missing in ~/.ibm/credentials.yaml.
COS resources are partitioned by storage instances, thus a user will have to provide one ahead of time.
COS' URIs must include a region, ergo some adjustments to repo's general code were necessary.
I've chosen rclone as a syncing/mounting tool. For each bucket an Rclone profile will be created under its name. If an existing profile with the same name exists, it will be deleted. Once a bucket is deleted, the profile associated with it will be removed as well.

Issue

I wasn't able to make persistent: False delete the used bucket, either after task concluded, or cluster terminated. Would appreciate some guidance.

Tested

test_ibm_storage_mounts
test_new_bucket_creation_and_deletion
test_bucket_bulk_deletion
test_bucket_external_deletion
test_upload_to_existing_bucket
test_list_source
test_private_bucket
test_nonexistent_bucket
test_invalid_names

notes on the rest of the tests

test_copy_mount_existing_storage - I believe it tests s3 exclusively.
test_public_bucket - can't provide a reliable public cos bucket.

landscapepainter

Hey @Cohen-J-Omer I left some comments. It should be ready after this. I'll let @romilbhardwaj sign off on this

landscapepainter · 2023-07-15T03:10:02Z

docs/source/getting-started/installation.rst

+
+Follow the next steps to create/extract the above fields:
+
+1. Create/Select a COS instance from the `web console <https://cloud.ibm.com/objectstorage/>`__.


@Cohen-J-Omer It would be sufficient to add an example in the yaml section starting with

# *** Copying files from IBM COS ***

just like how it's written for S3 and GCS

landscapepainter · 2023-07-15T03:21:05Z

sky/data/data_utils.py

 from multiprocessing import pool
+import concurrent.futures
 import os
 import subprocess
 from typing import Any, Callable, Dict, List, Optional, Tuple
 import urllib.parse
+import re
+import textwrap
+import threading
+from filelock import FileLock
+from enum import Enum

 from sky import exceptions


Suggested change

from multiprocessing import pool

import concurrent.futures

import os

import subprocess

from typing import Any, Callable, Dict, List, Optional, Tuple

import urllib.parse

import re

import textwrap

import threading

from filelock import FileLock

from enum import Enum

from sky import exceptions

import concurrent.futures

from enum import Enum

from multiprocessing import pool

import os

import subprocess

import textwrap

import threading

from typing import Any, Callable, Dict, List, Optional, Tuple

import urllib.parse

from filelock import FileLock

from sky import exceptions

1. Add process lock due to `subprocess_utils.run_in_parallel`. 2. Move locks to ibm adaptor.

Cohen-J-Omer · 2023-07-16T17:06:13Z

Hey @landscapepainter @romilbhardwaj ,
I've merged and added support for parallel storage deletion (subprocess_utils.run_in_parallel() in cli.storage_delete())

landscapepainter

@Cohen-J-Omer Thanks for adding support for parallel storage deletion! Would you mind merging master with the repo and confirm if it passes test_smoke.py? There was a recent update in test_multiple_buckets_creation_and_deletion.

romilbhardwaj · 2023-07-17T18:03:56Z

README.md

@@ -39,7 +39,7 @@ SkyPilot is a framework for running LLMs, AI, and batch jobs on any cloud, offer
 SkyPilot **abstracts away cloud infra burdens**:
 - Launch jobs & clusters on any cloud 
 - Easy scale-out: queue and run many jobs, automatically managed
- Easy access to object stores (S3, GCS, R2)
+- Easy access to object stores (S3, GCS, R2, IBM COS)


In the interest of keeping the README concise, can we exclude this change for now? We can add it when users start using it :)

You're breaking my heart here @romilbhardwaj...
Will remove.
If not all supported object stores are listed, I suggest implying there are more, e.g.:

Easy access to object stores, such as: S3, GCS, R2

romilbhardwaj · 2023-07-21T05:18:12Z

sky/data/data_utils.py

 from sky.utils import ux_utils

 Client = Any
-


Unintended?

Cohen-J-Omer · 2023-07-23T17:51:58Z

Hey @landscapepainter , @romilbhardwaj

I've Committed the requested changes.
Storage tests are executed successfully, including test_multiple_buckets_creation_and_deletion.

romilbhardwaj · 2023-07-23T23:47:53Z

Thanks @Cohen-J-Omer. I ran into an issue when running final tests:

I ran

(base) ➜  sky-experiments git:(storage-ibm) ✗ pytest tests/test_smoke.py::test_ibm_storage_mounts --ibm
bringing up nodes...
[ibm_storage_mounts] Test started. Log: less /var/folders/98/hhq8wrtx6y13196q61xphjsm0000gn/T/ibm_storage_mounts-5ifbcq3b.log
/Users/romilb/tools/anaconda3/lib/python3.9/multiprocessing/resource_tracker.py:216: UserWarning: resource_tracker: There appear to be 1 leaked semaphore objects to clean up at shutdown
  warnings.warn('resource_tracker: There appear to be %d '
[ibm_storage_mounts] Passed.
[ibm_storage_mounts] Log: less /var/folders/98/hhq8wrtx6y13196q61xphjsm0000gn/T/ibm_storage_mounts-5ifbcq3b.log
[ibm_storage_mounts] 
.
1 passed, 1152 warnings in 283.60s (0:04:43)

The test passed, but note the leaked semaphore warning. Now whenever I run any sky command, including sky status, I get the following warning:

(base) ➜  ~ sky status
Clusters
No existing clusters.

Managed spot jobs
No in progress jobs. (See: sky spot -h)
/Users/romilb/tools/anaconda3/lib/python3.9/multiprocessing/resource_tracker.py:216: UserWarning: resource_tracker: There appear to be 1 leaked semaphore objects to clean up at shutdown
  warnings.warn('resource_tracker: There appear to be %d '

If I switch branch to master, the warning goes away. Switching back to storage-ibm brings back the warning.

This is likely stemming from the global locks created in adaptors/ibm.py. Is there some way to move those locks out of global scope or define them lazily instead of eagerly?

For reference, I'm on M2 Macbook with python 3.9.13.

Also, can you merge with master?

Cohen-J-Omer · 2023-07-24T18:09:24Z

Hey @romilbhardwaj,

I can't replicate the issue. Seeing as it might be a RAM issue (from search results), I've ran the test on the poorest spec VM accessible to me, on a 3.9.17 venv (what's available on deadsnakes PPA), and on my personal Fedora 37 machine. Could it be a specific M2 and/or general Macbook issue?
I've merged with master and don't see Github conflicts alert.
Since the processes are spawned from Skypilot code, I can't create the locks there and pass a shared reference along, meaning the only other way for the processes to share a lock is to create the locks on the module level. Would love to get your input on the matter.

romilbhardwaj · 2023-07-24T19:30:28Z

I can't replicate the issue. Seeing as it might be a RAM issue (from search results), I've ran the test on the poorest spec VM accessible to me, on a 3.9.17 venv (what's available on deadsnakes PPA), and on my personal Fedora 37 machine. Could it be a specific M2 and/or general Macbook issue?

Hmm, I restarted my machine and have 17G memory free. The warning still shows up only when i checkout storage-ibm branch and run any skypilot command (i.e., when adaptors/ibm.py is imported). Works fine on master.

If I remove L24 ( threading_lock = threading.Lock()) from adaptors/ibm.py and run sky status, the error message goes away. Curiously, running an empty script with:

# test.py
import threading
threading_lock = threading.Lock()

# in shell
python test.py # No warning

does not trigger the warning.

BTW, do we need both multiprocessing_lock and threading_lock? Can we do with just multiprocessing_lock?

- drop multithread protection on cos client/resource

Cohen-J-Omer · 2023-07-26T16:11:15Z

Hey @romilbhardwaj,

Couldn't replicate the issue on any linux/python environment, ergo i couldn't debug the error you raised. From web searches and my inability to replicate the issue, I assume it's somehow connected to Mac machines, or specifically M1/ M2 processors.
I removed the threading_lock, after replacing threads on get_ibm_cos_bucket_region with processes, although it did serve a purpose.
Technical explanation:
get_ibm_cos_bucket_region used to take advantage of concurrency to speed up storage location lookups, since each lookup needs to reinitialize the boto3.client, which creates a not thread safe default boto3.session.
on I/O bound use-cases Multiprocess aren't the preferred solution (in contrast to threads) and will result on slower storage lookups on machines that have fewer cores.
The process lock is now lazily defined and initialized, as requested.

I hope we can now merge this PR.

landscapepainter

Hi @Cohen-J-Omer, I noticed that your recent merge with master branch does not reflect the update to the test I mentioned. The test is still outdated in this repository.

There was a recent update in test_multiple_buckets_creation_and_deletion.

Please merge with the recent master branch and check if the updated test passes.

With regard to the error @romilbhardwaj encountered, I ran pytest tests/test_smoke.py::test_ibm_storage_mounts --ibm on my machine, Debian 4.19.260-1 (2022-09-29) x86_64 GNU/Linux, with python 3.8 and did not come across the same error. Seems like it's an issue for M1/M2 machines:

https://stackoverflow.com/questions/72435382/how-can-i-fix-1-leaked-semaphore-objects-to-clean-up-at-shutdown-error-on-mac

…e-ibm

Cohen-J-Omer · 2023-07-27T13:36:51Z

Hey @landscapepainter, @romilbhardwaj:
Merged with master. All (IBM related) tests passed.
Thanks for checking out the error @landscapepainter.

romilbhardwaj

Thanks @Cohen-J-Omer - all looks good! I tested by installing from scratch in a docker container (both, pip install -e .[ibm] and pip install -e .[aws]). This should be ready to go. Thanks again for the hard work!

romilbhardwaj · 2023-07-30T00:23:14Z

tests/test_smoke.py

+from sky.adaptors import ibm
+from sky.data.data_utils import Rclone


nit: alphabetical ordering for imports

Done. Hope I understood you, since i had to reorder 2 other import lines in the process.

Cohen-J-Omer added 30 commits April 23, 2023 14:06

added required ibm references

9b8d98f

support for ibm authentication and vpc client

bb73d57

marked places first requiring CLI implementation

2d477e3

node_provider implementation and template file

c63cd83

IBM's cloud and catalog implementations for sky

de6ba55

removed redundant check: verified in check.py

2ebbef5

general updates to node provider

2a37551

vpc_provider initial integration

2d13b26

ibm cloud class update

82c7d22

update authentication process

074ec93

update ibm cloud adapter

37a35e1

remove global vpc client

d3dc988

small fixes and updates

5195f2e

major update - vpc provider

1266f6e

further adjust node provider to vpc provider

bb8bffd

authentication updates relevant file mounts

e29bb4b

add search and tagging clients to ibm adaptor

ca122a2

support for --r in sky status when cluster stopped

8530e74

beginning support for a separate worker profile

bc3d1cd

update ibm specific libraries

662e702

support tagging for cluster's vpc

c63fc5d

separate profile for workers on config file

98539a3

remove redundant constants (due to vpc tags)

9d52cf2

vpc now terminates after head node

ae7ca55

enable vpc termination when cluster is stopped

ffdb2c9

update block_list upon failover

bf69f54

skypilot adjustment - attach ip to all nodes

e83bd5f

update image to ubuntu 22-4

100a45d

replace mockup csv with default data frame

a0a83d8

small update to vpc provider

4773565

remove redundant pylint remarks

60fa793

landscapepainter reviewed Jul 15, 2023

View reviewed changes

Cohen-J-Omer added 5 commits July 16, 2023 15:33

URL storage instructions for IBM COS.

aee730e

reorder import statements

51a7df1

Merge remote-tracking branch 'skypilot/master' into storage-ibm

bf91dc5

Adjust thread&process safe sync mechanisms:

ccb4fdf

1. Add process lock due to `subprocess_utils.run_in_parallel`. 2. Move locks to ibm adaptor.

code reformatting

7341a6b

Cohen-J-Omer added 2 commits July 17, 2023 14:56

Include IBM COS on README.md

a6748b0

Update docs for ibm cos clients' functions

53922f5

landscapepainter reviewed Jul 20, 2023

View reviewed changes

romilbhardwaj reviewed Jul 21, 2023

View reviewed changes

Cohen-J-Omer added 3 commits July 23, 2023 13:29

reformatting

cda56f0

updated README

ff1b33d

IBM support multiple_buckets_creation_and_deletion

c24bd13

Cohen-J-Omer added 2 commits July 26, 2023 18:45

multiprocess support for get_ibm_cos_bucket_region

9f2de17

- lazily define&init global process lock

e7e0696

- drop multithread protection on cos client/resource

landscapepainter reviewed Jul 26, 2023

View reviewed changes

Merge branch 'master' of github.com:skypilot-org/skypilot into storag…

02ff7d9

…e-ibm

romilbhardwaj approved these changes Jul 30, 2023

View reviewed changes

Cohen-J-Omer added 2 commits July 30, 2023 11:33

add code docs explaining the use of processes

ac35262

import reordering

674ccbd

romilbhardwaj merged commit 657fd84 into skypilot-org:master Aug 1, 2023
14 of 16 checks passed

landscapepainter mentioned this pull request Aug 25, 2023

[Docs/IBM] Missing instructions for setting up IBM credentials #1902

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration of IBM's Cloud Object Storage: COS #1966

Integration of IBM's Cloud Object Storage: COS #1966

Cohen-J-Omer commented May 18, 2023 •

edited

Loading

landscapepainter left a comment

landscapepainter Jul 15, 2023

landscapepainter Jul 15, 2023

Cohen-J-Omer commented Jul 16, 2023

landscapepainter left a comment

romilbhardwaj Jul 17, 2023

Cohen-J-Omer Jul 23, 2023

romilbhardwaj Jul 21, 2023

Cohen-J-Omer commented Jul 23, 2023

romilbhardwaj commented Jul 23, 2023 •

edited

Loading

Cohen-J-Omer commented Jul 24, 2023

romilbhardwaj commented Jul 24, 2023

Cohen-J-Omer commented Jul 26, 2023 •

edited

Loading

landscapepainter left a comment •

edited

Loading

Cohen-J-Omer commented Jul 27, 2023 •

edited

Loading

romilbhardwaj left a comment

romilbhardwaj Jul 30, 2023

Cohen-J-Omer Jul 30, 2023


		Follow the next steps to create/extract the above fields:

		1. Create/Select a COS instance from the `web console <https://cloud.ibm.com/objectstorage/>`__.

		from sky.adaptors import ibm
		from sky.data.data_utils import Rclone

Integration of IBM's Cloud Object Storage: COS #1966

Integration of IBM's Cloud Object Storage: COS #1966

Conversation

Cohen-J-Omer commented May 18, 2023 • edited Loading

Support for IBM's COS.

Notes:

Issue

Tested

notes on the rest of the tests

landscapepainter left a comment

Choose a reason for hiding this comment

landscapepainter Jul 15, 2023

Choose a reason for hiding this comment

landscapepainter Jul 15, 2023

Choose a reason for hiding this comment

Cohen-J-Omer commented Jul 16, 2023

landscapepainter left a comment

Choose a reason for hiding this comment

romilbhardwaj Jul 17, 2023

Choose a reason for hiding this comment

Cohen-J-Omer Jul 23, 2023

Choose a reason for hiding this comment

romilbhardwaj Jul 21, 2023

Choose a reason for hiding this comment

Cohen-J-Omer commented Jul 23, 2023

romilbhardwaj commented Jul 23, 2023 • edited Loading

Cohen-J-Omer commented Jul 24, 2023

romilbhardwaj commented Jul 24, 2023

Cohen-J-Omer commented Jul 26, 2023 • edited Loading

landscapepainter left a comment • edited Loading

Choose a reason for hiding this comment

Cohen-J-Omer commented Jul 27, 2023 • edited Loading

romilbhardwaj left a comment

Choose a reason for hiding this comment

romilbhardwaj Jul 30, 2023

Choose a reason for hiding this comment

Cohen-J-Omer Jul 30, 2023

Choose a reason for hiding this comment

Cohen-J-Omer commented May 18, 2023 •

edited

Loading

romilbhardwaj commented Jul 23, 2023 •

edited

Loading

Cohen-J-Omer commented Jul 26, 2023 •

edited

Loading

landscapepainter left a comment •

edited

Loading

Cohen-J-Omer commented Jul 27, 2023 •

edited

Loading