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

fix env setting for remote_storage_controller #425

Draft
wants to merge 2 commits into
base: develop-cloud
Choose a base branch
from

Conversation

itutu-tienday
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@milesAraya milesAraya left a comment

Choose a reason for hiding this comment

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

念の為に.env.exampleにAWS_ACCESS_KEY_IDを書いた方がいいと思いませんか?

@milesAraya milesAraya marked this pull request as draft August 29, 2024 16:18
@milesAraya milesAraya self-requested a review August 29, 2024 16:19
@milesAraya milesAraya changed the title fix env setting example for remote_storage_controller fix env setting for remote_storage_controller Aug 29, 2024
Copy link
Collaborator Author

@itutu-tienday itutu-tienday left a comment

Choose a reason for hiding this comment

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

@milesAraya
comments noted.

- aws cli (`aws s3 ls`) も、基本的には Server(AWS) Side のファイルリストfilterには非対応だが、
`aws s3 sync` の利用により、間接的に Server(AWS) Side での filterが利用可能
- なお aws cli の利用は暫定的な対応であり、最終的には boto3 でfilterを実現できることが望ましい
Download all experiment metadata files from S3 using boto3.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason for using aws s3 sync is, as stated in the comment, “to have the filter performed on the Server (AWS) Side”.

Since optinist may generate a large number of files, I care about avoiding useless filters on the client side.

From the above, it is better to estimate the maximum number of files that can actually be generated and compare the performance between the aws s3 sync version and the boto3 version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

どちらの方法がいいかclaude.aiに聞いたんですが、この通りで答えました。boto3を使用した改訂版がAWS CLIのs3 syncよりも優れている理由は以下の通りです:

  1. 依存関係の削減:boto3はPythonのAWS SDKであり、追加のCLIツールをインストールする必要がありません。これにより、システムの依存関係が減少し、環境の管理が容易になります。
  2. セキュリティの向上:boto3はプログラム内で直接AWS認証情報を管理できるため、シェルコマンドで認証情報を扱うよりも安全です。
  3. パフォーマンスの最適化:boto3を使用すると、必要なファイルのみを効率的にフィルタリングし、ダウンロードすることができます。これにより、不要なネットワークトラフィックを削減できる可能性があります。
  4. エラーハンドリングの改善:boto3を使用すると、Pythonのtry-except文を用いて、きめ細かいエラーハンドリングが可能になります。これにより、問題が発生した際の対応がより柔軟になります。
  5. プログラマティックな制御:boto3を使用すると、S3操作をよりきめ細かく制御でき、アプリケーションのニーズに合わせてカスタマイズしやすくなります。
    これらの理由により、boto3を使用した改訂版は、多くの場合においてAWS CLIのs3 syncコマンドよりも優れた選択肢となり得ます。

もちろん、川田さんの判断にお任せしています

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

はい、基本的には外部コマンドに依存せず、pythonコードで完結できる形が望ましいと思います。
しかし、boto3(S3 API)が、aws s3 sync に相当する機能を提供していないため、まずは aws s3 sync を利用した経緯です。

先のメッセージで記載したように、性能比較も行い、方式をFixできると良いと考えます。

※claudeの回答のうち、# 3 などは、aws cliでも同等そうですね。

- aws cli (`aws s3 ls`) も、基本的には Server(AWS) Side のファイルリストfilterには非対応だが、
`aws s3 sync` の利用により、間接的に Server(AWS) Side での filterが利用可能
- なお aws cli の利用は暫定的な対応であり、最終的には boto3 でfilterを実現できることが望ましい
Download all experiment metadata files from S3 using boto3.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For revisions that differ from the PR in content, please create a separate PR.
*For now, let's rename this PR, shall we?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I thought I could add here because the description "fix env setting for remote_storage_controller" was close to the changes i made to use boto3. However, if you don't want to go with this approach, we can delete this last PR.

@itutu-tienday
Copy link
Collaborator Author

@milesAraya

念の為に.env.exampleにAWS_ACCESS_KEY_IDを書いた方がいいと思いませんか?

実際の動作環境(AWS上)では、セキュリティ情報は .env には記載しませんが、開発時には .env に設定する形でも良いですね。

  • FYI) confidential infos on application
    • DB connection information
    • Firebase access information
    • AWS access information
    • etc.

@milesAraya
Copy link
Collaborator

milesAraya commented Aug 30, 2024

実際の動作環境(AWS上)では、セキュリティ情報は .env には記載しませんが、開発時には .env に設定する形でも良いですね。

  • FYI) confidential infos on application

    • DB connection information
    • Firebase access information
    • AWS access information
    • etc.

Do you have an idea how we can improve the consistency of communication with AWS CLI? For me it worked sometimes and then did not work other times. I am not sure what the issue was, but sometimes I cannot log in because the AWS CLI has not correctly shared the Keys

@itutu-tienday
Copy link
Collaborator Author

@milesAraya

Do you have an idea how we can improve the consistency of communication with AWS CLI? For me it worked sometimes and then did not work other times. I am not sure what the issue was, but sometimes I cannot log in because the AWS CLI has not correctly shared the Keys

I recognize that any combination of AWS CLI / boto3, aws confingure / env variables will basically work.
Let's discuss the pattern that it doesn't work, and we'll share our understanding of the pattern at a later date, e.g. through screen sharing.

@milesAraya milesAraya requested review from milesAraya and removed request for milesAraya August 30, 2024 10:01
@itutu-tienday
Copy link
Collaborator Author

@milesAraya

Some of the issues raised in this PR have already been addressed in the following PRs. I hope you will review the following PR first.

@itutu-tienday itutu-tienday linked an issue Sep 17, 2024 that may be closed by this pull request
3 tasks
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.

[Cloud Support] Setup development environment
2 participants