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

Feature: SCP (Samsung Cloud Platform) Support Contribution #926

Merged
merged 6 commits into from
Jan 9, 2024

Conversation

sangjun-kang
Copy link
Contributor

Overview

This pull request adds support for SCP (Samsung Cloud Platform) by implementing the necessary code. I have completed the development and testing phases and followed all the procedures outlined in the contributing guide.

Details

Progress

  • Code development and testing completed
  • Issue and discussion registration
  • Code review and adjustments

@sarahwooders sarahwooders self-requested a review December 7, 2023 21:17
@sarahwooders
Copy link
Contributor

@sangjun-kang could you please fix the formatting checks? You can run it with:

pip install -r requirements-dev.txt
black -l 140 .
pytype --config .pytype.cfg skyplane
autoflake --in-place --remove-all-unused-imports --remove-unused-variables --recursive skyplane

@sangjun-kang
Copy link
Contributor Author

@sangjun-kang could you please fix the formatting checks? You can run it with:

pip install -r requirements-dev.txt
black -l 140 .
pytype --config .pytype.cfg skyplane
autoflake --in-place --remove-all-unused-imports --remove-unused-variables --recursive skyplane

Hello @sarahwooders,

It seems that some parts were missed during the execution of the black formatting.
I have re-run the checks, and everything should be in order now.
Please review the changes, and let me know if there are any further adjustments needed.

Thank you!

@sarahwooders
Copy link
Contributor

Thank you! Are there also any additional dependencies needed for SCP? If so, could you please update https://github.com/skyplane-project/skyplane/blob/main/pyproject.toml

@sangjun-kang
Copy link
Contributor Author

sangjun-kang commented Dec 19, 2023

Thank you! Are there also any additional dependencies needed for SCP? If so, could you please update https://github.com/skyplane-project/skyplane/blob/main/pyproject.toml

Thank you for your comment! I've confirmed, and SCP doesn't require any additional dependencies beyond boto3.
I've updated the pyproject.toml to reflect the SCP section.
I appreciate your attention to detail. If you have any more questions or concerns, feel free to let me know!

@sangjun-kang
Copy link
Contributor Author

Sorry, Sarah. The authentication key shared by Suk-hoon had IP restrictions (for security reasons). Suk-hoon has removed the IP restrictions, so please test it again. Also, there is authentication key information in the logs, so it would be great if you could erase the logs.

@SukHoon-Jung
Copy link

nk you for your comment! I've confirmed, and SCP doesn't require any additional dependencies beyond boto3.
I've updated the pyproject.toml to reflect the SCP section.
I appreciate your attention to detail. If you have any more questions or concerns, feel free to let me kno

@sarahwooders, @sangjun-kang , This error is due to the security settings of my account. sorry.
I've changed the settings so you can access now.

By the way, you mentioned that there was an error in the credential format.
could you pinpoint where the error occurs?

@sarahwooders
Copy link
Contributor

I just deleted the comment, and the skyplane init command worked now, thanks!

@sangjun-kang
Copy link
Contributor Author

@sarahwooders, I encountered errors in the following files when running pytype --config .pytype.cfg skyplane, but these are not files I have committed, so I couldn't address them. Please take a look:

skyplane/gateway/gateway_queue.py
skyplane/gateway/operators/gateway_receiver.py
skyplane/gateway/gateway_daemon_api.py
skyplane/obj_store/hdfs_interface.py
skyplane/obj_store/r2_interface.py
skyplane/cli/cli_cloud.py
skyplane/gateway/operators/gateway_operator.py
skyplane/cli/impl/progress_bar.py
skyplane/planner/planner.py

@sarahwooders
Copy link
Contributor

@sangjun-kang sorry for the delayed response, have been traveling! I can take a look at the pytest issues this week.

@sarahwooders
Copy link
Contributor

sarahwooders commented Jan 2, 2024

Could you run poetry install --with dev and poetry run black -l 140 .? I can also fix the formatting if you give me push permissions on your repo. On my end, it only changed two files.

@sangjun-kang
Copy link
Contributor Author

Could you run poetry install --with dev and poetry run black -l 140 .? I can also fix the formatting if you give me push permissions on your repo. On my end, it only changed two files.

Hi @sarahwooders

I made changes to the SCP Object Storage API yesterday, adjusting some source code. It seems black formatting was missed, so I ran it again and committed the changes.
And I've granted you repo access. Let me know if you need anything else.

Thanks,

Copy link
Contributor

@sarahwooders sarahwooders left a comment

Choose a reason for hiding this comment

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

This now works for me - thank you so much for the contribution! I'll merge once the tests re-run.

skyplane/cli/cli_init.py Show resolved Hide resolved
@sarahwooders sarahwooders merged commit 4602d9c into skyplane-project:main Jan 9, 2024
11 checks passed
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