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

Upload Byte data #2196

Open
wants to merge 3 commits into
base: dev/v0.7.0
Choose a base branch
from

Conversation

bhargav191098
Copy link
Contributor

This PR will enable uploading byte data through fedml storage upload command.

@bhargav191098 bhargav191098 self-assigned this Jun 21, 2024
@bhargav191098 bhargav191098 changed the base branch from master to dev/v0.7.0 June 21, 2024 03:08
@alaydshah
Copy link
Contributor

To summarize, we should try to make following modifications:

  1. Figure out a way to eliminate the byte data type flag and just recognize the type internally.
  2. Restructure code a bit to avoid long functions with many if-else conditions. We can make it more readable by having more manageable sub functions for different data type and centralize the common logic in main function and call respective sub-function based on the detected data type

@@ -92,7 +67,8 @@ def upload(data_path, api_key, name, description, tag_list, service, show_progre
}

try:
response = _create_dataset(api_key=api_key, json_data=json_data)
response = _create_dataset(api_key=api_key, json_data=json_data, encrypted_api_key_flag=encrypted_api_key_flag)
print("create dataset ", response)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need to print here?

@@ -120,7 +96,7 @@ def download(data_name, api_key, service, dest_path, show_progress=True) -> FedM
download_url = metadata.download_url
given_extension = os.path.splitext(data_name)[1]
is_file = True
if(given_extension is None or given_extension ==""):
if (given_extension is None or given_extension == ""):
Copy link
Contributor

@alaydshah alaydshah Jun 22, 2024

Choose a reason for hiding this comment

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

if not given_extension:

@alaydshah
Copy link
Contributor

alaydshah commented Jun 23, 2024

Please remember to always propogate the newly added variables all the way up to CLIs and APIs. This is the entrypoint, and we don't have the byte_data property wired in here. Always good to test it from end user perspective so we can catch such obvious slip-ups.

image

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.

2 participants