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

new upload procedure #789

Merged
merged 12 commits into from
Jan 12, 2022
Merged

new upload procedure #789

merged 12 commits into from
Jan 12, 2022

Conversation

pkasprzyk
Copy link
Contributor

No description provided.

@pkasprzyk pkasprzyk marked this pull request as draft December 21, 2021 10:28
@pkasprzyk pkasprzyk force-pushed the pk/new-upload branch 2 times, most recently from 15f5be2 to 2e55cbd Compare December 21, 2021 14:43
@pkasprzyk pkasprzyk marked this pull request as ready for review January 11, 2022 12:56
@pkasprzyk pkasprzyk changed the title WIP new upload procedure new upload procedure Jan 11, 2022
@@ -15,6 +15,16 @@
#


class MultipartConfig:
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be a dataclass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's Legacy Client so I didn't want to introduce dataclasses there. But I'm not sure why 🤣

Done.


from future.builtins import object

from neptune.internal.storage.storage_utils import (
UploadEntry,
AttributeUploadConfiguration,
)
from neptune.new.exceptions import InternalClientError
from neptune.new.internal.backends.api_model import MultipartConfig


class FileChunk(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be a dataclass as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really my code, but you're right :)

Copy link
Contributor

Choose a reason for hiding this comment

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

You touched this, it's on you :p

self._max_chunk_size = multipart_config.max_chunk_size
self._max_chunk_count = multipart_config.max_chunk_count

def _get_chunk_size(self) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add info here that our aim is to maximize number of chunks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it, actually? That's just my implementation decision ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

if isinstance(chunk, str):
chunk = chunk.encode("utf-8")
new_offset = last_offset + len(chunk)
yield FileChunk(chunk, last_offset, new_offset)
Copy link
Contributor

Choose a reason for hiding this comment

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

Make it kwargs, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

def upload_file_attribute(
swagger_client: SwaggerClient,
container_id: str,
attribute: str,
source: Union[str, bytes],
ext: str,
) -> Optional[NeptuneException]:
multipart_config: Optional[MultipartConfig],
) -> List[NeptuneException]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a situation when we send more than one exception?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we just want to make it consistent with upload_file_set_attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly that, I wanted consistency

shnela
shnela previously approved these changes Jan 12, 2022
@pkasprzyk pkasprzyk merged commit a2077a4 into master Jan 12, 2022
@pkasprzyk pkasprzyk deleted the pk/new-upload branch January 12, 2022 14:49
pkasprzyk pushed a commit that referenced this pull request Jan 13, 2022
pkasprzyk added a commit that referenced this pull request Jan 13, 2022
* fix changelog for #789

* make sure file names are really unique

* neptune.ai token in old client tests

* stop using os.getrandom

* use new token in new tests, too :-]
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