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

Issue/zip operation #787

Merged
merged 33 commits into from
Oct 19, 2024
Merged

Issue/zip operation #787

merged 33 commits into from
Oct 19, 2024

Conversation

4nshuman
Copy link
Contributor

@4nshuman 4nshuman commented Oct 5, 2024

This PR, when complete, will solve the issue #116

@jelveh
Copy link
Contributor

jelveh commented Oct 5, 2024

Thanks again for taking care of this issue. A lot of users will appreciate this feature being implemented quickly. I'll review this soon.

@jelveh
Copy link
Contributor

jelveh commented Oct 6, 2024

Testing this right now. The first pass looks amazing! Will circle back with more testing.

@4nshuman
Copy link
Contributor Author

4nshuman commented Oct 6, 2024

The unzip functionality is also pushed, let me know in case you find something that can be optimised.

I could use some help on the percentage calculation for the progress window, current iteration runs on a couple of assumptions. Let me know if you have some ideas on a better implementation.

@jelveh
Copy link
Contributor

jelveh commented Oct 7, 2024

This is pretty awesome. I still haven't found issues! Could you elaborate on the percentage calculation? What can I help you with? it seems to be working well.

@4nshuman
Copy link
Contributor Author

4nshuman commented Oct 7, 2024

@jelveh
So, at the moment, apart from the zipping progress dialog, there's only 1 other progress dialog that shows percentage & that's the upload progress dialog box.
image
Now the upload dialog box is able to show the progress without any calculation on the helper js because the Puter SDK's file sub-system has upload.js laced with the progress calculation:
image
This is pretty awesome, because the upload module would be able to check what's the total file size, and how much of it has been uploaded and thereby it would return us a percentage to show on our dialog boxes(The documentations do not mention it, I think it should, but that's a whole another conversation ! :D).

Now this progress counter option is currently not supported on the write.js and the read.js from the file sub-system on the Puter SDK. Now that means we'll have to calculate the percentage of file read and written on the fly within the helper module itself and that calculation comes with some assumptions, since we cannot calculate the exact percentage while the operation is in progress. We need to wait for a read/write operation to complete and then compute the percentage.
for eg : we are writing 4 files in total, and currently we are on the 2nd file being written, in the case described above we would not be able to show the precise percentage of 35%((25% for 100% write completion on 1st file) + (10% for 2.5% write completion on 2nd file)). We will only be able to show 25% then 50% then 75% and finally 100%, nothing in between.

This same issue persists with the fflate/JSzip/Archiver libraries as well, they do not have an internal progress counter, so we need to do the calculation for them.

So all that being the preface, I came up with some percentage calculations of my own :
image

I assume that converting the blobs to uInt8Array would take somewhere up-to 60% then zipping would take another ~23% and writing another ~14%. This leaves a bit of buffer in case things got slowed down and took longer than expected.

Do you think we could better implement this somehow to get rid of the assumption ?
If not, we can keep these values in until we develop the progress counter on write and read modules of Puter SDK file sub-system. Note that even then we'll have to keep some assumption for the fflate's zipping operation.

TLDR : there's no progress counter on write.js and read.js just like the one that exists on upload.js. So I am calcualting the percantage manually on helper for these 2 operations(read and write)

P.S : Apologies for the really long description on this, please feel free to reach out to me in case of any queries.

@jelveh
Copy link
Contributor

jelveh commented Oct 7, 2024

Thank you for the detailed response. Come to think of it, I used a spinner because I didn't have time to implement a progress bar for zipping. Very glad you picked it up!

I need to think about this a little more and will get back to you soon.

Thanks again.

@4nshuman
Copy link
Contributor Author

4nshuman commented Oct 8, 2024

@jelveh
I've been studying the Puter codebase and have been enjoying working on the tool so far.
I'd love to pick up the progress calculation implementation on the read and write modules from the fs-submodule.
Give it a thought and let me know would be glad to contribute and help out.

@jelveh
Copy link
Contributor

jelveh commented Oct 8, 2024

Thank you @4nshuman. That would be awesome! Do you think you could implement progress bar for the copy/move operations? This is something users really wanted but we haven't gotten around to implementing yet. Let me know and I'll create an issue. If you want to go for read and write, we could do that too!

@jelveh
Copy link
Contributor

jelveh commented Oct 8, 2024

@4nshuman upon further testing (before I get to progress), this looks really good. I haven't found any bugs yet!

There is one important optimization note though. Oftentimes users may want to unzip directories with many subdirectories and files, that's why it would be more optimized to use the puter.fs.upload method since it takes care of batching the request internally. This is important since sending many concurrent requests can severely impact the server, whereas the batch endpoint can easily handle thousands of uploads.

Let me know your thoughts or if you need help with this.

@4nshuman
Copy link
Contributor Author

4nshuman commented Oct 9, 2024

Hey @jelveh ,
Yes, please provide me an issue to implement the progress on write and read, would be happy to work on it.

About the optimization, So If I am understanding this correctly, you would like us to use the upload module to write the files instead of manually triggering the write module correct ?
If not could you please elaborate a bit on how exactly you're planning on using the upload here ?

@jelveh
Copy link
Contributor

jelveh commented Oct 11, 2024

Yes, please provide me an issue to implement the progress on write and read, would be happy to work on it.

Will do! Thanks again :)

So If I am understanding this correctly, you would like us to use the upload module to write the files instead of manually triggering the write module correct ?

For cases such as unzipping, upload works better because it has built-in support for batching writes and mkdirs. It's extremely fast for writing many files and directories (which happens a lot for unzipping).

I tested your code a lot and it's working beautifully! I look forward to merging and pushing to prod soon ✌️

@4nshuman
Copy link
Contributor Author

4nshuman commented Oct 11, 2024

For cases such as unzipping, upload works better because it has built-in support for batching writes and mkdirs. It's extremely fast for writing many files and directories (which happens a lot for unzipping).

This needs me to convert the incoming unzipped object to a list of DirectoryEntries and Files since upload module uses this list to create the actual files. I'll need some time to work on this, will update back soon.

I tested your code a lot and it's working beautifully! I look forward to merging and pushing to prod soon ✌️

Looking forward to it @jelveh !

@4nshuman
Copy link
Contributor Author

Hey @jelveh , @KernelDeimos,
As discussed I've connected the upload module to the zip write functionality to utilise the batch processing from upload module.
Also connected the inbuilt progress counter from upload module to the zip functionality.
I am pretty confident that the feature should not break(tried my best to break and succeeded.. then fixed the broken stuff :P)
Please give it a go and let me know in case you find any anomalies in the functionality.

@4nshuman 4nshuman marked this pull request as ready for review October 19, 2024 20:40
@jelveh
Copy link
Contributor

jelveh commented Oct 19, 2024

woohooo! Testing now 🔥

@jelveh jelveh merged commit 90e7098 into HeyPuter:main Oct 19, 2024
4 checks passed
@jelveh
Copy link
Contributor

jelveh commented Oct 19, 2024

Beautiful! Thank you @4nshuman for the amount of time and effort you put into this awesome PR. Users will deeply appreciate this reimplementation.
Can't wait to deploy it to Puter.com as well! 🔥

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