Ipfs file upload progress #604

dryajov · 2017-10-02T00:54:30Z

Adding this as we need this ASAP, should address #596 (comment)

Passes the progress bar option for tranfers over http. Progress is shown for your payload being streamed upto the server. There is a pause. Then you get your list of files.

* support support specify hash alg * Add test for add with --hash option. Pass raw cid/multihash to ipfs object get/data * Allow object get/data to accept CID * Var naming tweaks from review

dryajov · 2017-10-02T00:55:24Z

pinging @skzap. This supports the progress flag for upload progress.

codecov · 2017-10-02T01:01:31Z

Codecov Report

Merging #604 into master will increase coverage by 0.31%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #604      +/-   ##
=========================================
+ Coverage   83.08%   83.4%   +0.31%     
=========================================
  Files         106     107       +1     
  Lines        1460    1482      +22     
=========================================
+ Hits         1213    1236      +23     
+ Misses        247     246       -1

Impacted Files	Coverage Δ
src/files/add.js	`89.65% <100%> (+0.36%)`	⬆️
src/utils/request-api.js	`93.87% <100%> (+1.83%)`	⬆️
src/utils/progress-stream.js	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38d7289...9a11a83. Read the comment docs.

dryajov · 2017-10-02T04:49:39Z

Seems like firefox is timing out on travis, can someone with write access rerun the build?

lidel · 2017-10-02T05:51:01Z

examples/upload-file-via-browser/package.json

@@ -13,7 +13,7 @@
  "devDependencies": {
    "babel-core": "^5.4.7",
    "babel-loader": "^5.1.2",
-    "ipfs-api": "^12.1.7",
+    "ipfs-api": "../../",


That's an example in the examples directory. Just repointed it to use the current ipfs-api rather than a published version.

Thanks, now it makes sense 👍

victorb · 2017-10-05T11:47:44Z

test/files.spec.js

+        expect(err).to.not.exist()
+
+        expect(res).to.have.length(1)
+        expect(progress).to.be.greaterThan(0)


Should exactly be 100 right?

agreed with @victorbjelkholm

daviddias · 2017-10-06T12:29:35Z

@dryajov have you tried using this with js-ipfs and see if it works for the CLI?

daviddias · 2017-10-06T12:34:51Z

test/files.spec.js

+        expect(err).to.not.exist()
+
+        expect(res).to.have.length(1)
+        expect(progress).to.be.greaterThan(0)


agreed with @victorbjelkholm

victorb · 2017-10-06T12:44:06Z

test/files.spec.js

+
+        expect(res).to.have.length(1)
+        expect(progress).to.be.equal(100)
+        expect(progressCount).to.be.greaterThan(0)


Should not have to use greaterThan here. We should know how many times progressHandler is being called, and assert on that exactness. Not "something something above 0".

daviddias · 2017-10-06T12:46:19Z

@dryajov just checked and the value being returned is always 12

AssertionError: expected 12 to equal 100

Which means that go-ipfs returns an in progress, but it is not clear what this number is. Also, this feature needs to be tested with bigger files, directories and be documented.

In order to push this forward, we need:

Document this feature in spec https://github.com/ipfs/interface-ipfs-core/tree/master/API/files#files-api
Create big file and directory tests
Move tests to https://github.com/ipfs/interface-ipfs-core/tree/master/src
Test against js-ipfs too
Make sure that when linked, js-ipfs CLI picks up on this to show the progress bar.

@dryajov up to finish this PR?

dryajov · 2017-10-06T13:04:40Z

Sure thing - I can finish this.

dryajov · 2017-10-06T14:22:14Z

@diasdavid The cli works with - ipfs/js-ipfs#994

dryajov · 2017-10-06T14:28:19Z

Move tests to https://github.com/ipfs/interface-ipfs-core/tree/master/src

~~Which tests do we want to move? All the progress ones or is that supposed to read add tests to interface-ipfs-core~~

I just checked the core tests - I know what this means now

dryajov · 2017-10-06T16:27:40Z

Added interface tests - ipfs-inactive/interface-js-ipfs-core#155

dryajov · 2017-10-06T16:40:36Z

Re: documenting feature - the docs for .add state that the options object is an expansion of https://github.com/ipfs/js-ipfs-unixfs-engine#importer-api object, which already documents this functionality:

progress (function): a function that will be called with the byte length of chunks as a file is added to ipfs.

dryajov · 2017-10-06T19:32:23Z

update - I believe I have all the todos covered, except the Test against js-ipfs too.

I can run the js-ipfs manually and it works, but the tests are for some reason not picking up the flag, or something else is going on - I get no progress output in stdout.

daviddias · 2017-10-09T13:34:15Z

Thanks for pushing this, @dryajov. See review on ipfs-inactive/interface-js-ipfs-core#155. Also, you skipped updating the docs.

dryajov · 2017-10-09T15:41:25Z

@diasdavid RE docs: #604 (comment)

This is what I'm talking about - the docs just link to the DAG importer options, so I don't think I skipped the docs :)

options is an optional object argument containing the DAG importer options.

daviddias · 2017-10-09T17:15:34Z

@dryajov good catch. That is confusing for users though. Do you mind pulling those supported options to the API doc?

daviddias · 2017-10-12T12:15:37Z

@dryajov what about calling the callback with the error?

dryajov · 2017-10-12T16:17:18Z

the problem is that the callback was already called, the only thing thats left is the stream and the through processors :(

I somehow need to make that error propagate all the way to - https://github.com/ipfs/js-ipfs-api/blob/10b0bde35a2dc106b93d52057b91265cb73a4614/src/files/add.js#L50 where that callback should get resolved once processing is finished.

dryajov · 2017-10-15T00:47:29Z

docs have also been updated in - https://github.com/ipfs/interface-ipfs-core/pull/155/files#diff-d1498a38c27c6500d8aa28946eca0d2cR29

dryajov · 2017-10-15T00:59:20Z

This issue is ready to ship. However, the changes in ipfs/js-ipfs#1036, currently prevent propagating errors correctly, fortunately, this should be extremely rare/catastrophic errors, i.e. out of disk space on the remote ipfs node. In order to get this working, some reworking of how responses are processed might be required, I'll open a separate issue to track that effort, but we should not hold this issues because of it.

daviddias · 2017-10-18T09:32:01Z

@dryajov tests fail locally:

Summary of all failing tests
 FAIL  test/interface/files.spec.js (5.801s)
  ● .files › callback API › .add › BIG buffer with progress

    AssertionError: expected 448324032 to equal 15000000

      at ipfs.files.add (node_modules/interface-ipfs-core/src/files.js:143:33)
      at data (src/utils/stream-to-value.js:12:18)
      at ConcatStream.<anonymous> (node_modules/concat-stream/index.js:36:43)
      at emitNone (events.js:110:20)
      at ConcatStream.emit (events.js:207:7)
      at finishMaybe (node_modules/readable-stream/lib/_stream_writable.js:607:14)
      at endWritable (node_modules/readable-stream/lib/_stream_writable.js:615:3)
      at ConcatStream.Object.<anonymous>.Writable.end (node_modules/readable-stream/lib/_stream_writable.js:571:41)
      at DAGNodeStream.onend (node_modules/readable-stream/lib/_stream_readable.js:570:10)
      at Object.onceWrapper (events.js:314:30)

  ● .files › callback API › .add › add a nested dir as array with progress

    AssertionError: expected 1386840 to equal 600408

      at ipfs.files.add (node_modules/interface-ipfs-core/src/files.js:230:33)
      at data (src/utils/stream-to-value.js:12:18)
      at ConcatStream.<anonymous> (node_modules/concat-stream/index.js:36:43)
      at emitNone (events.js:110:20)
      at ConcatStream.emit (events.js:207:7)
      at finishMaybe (node_modules/readable-stream/lib/_stream_writable.js:607:14)
      at endWritable (node_modules/readable-stream/lib/_stream_writable.js:615:3)
      at ConcatStream.Object.<anonymous>.Writable.end (node_modules/readable-stream/lib/_stream_writable.js:571:41)
      at DAGNodeStream.onend (node_modules/readable-stream/lib/_stream_readable.js:570:10)
      at Object.onceWrapper (events.js:314:30)

Am I missing something?

dryajov · 2017-10-18T10:10:38Z

@diasdavid

There appears to be weirdness with how go-ipfs reports progress.

For files, it will report the total bytes transferred so far
For directories, the file sizes are reported for each file uploaded, in addition to that, it appears that extra bytes are being reported as well.

Here is some quick output I added to the test to track file sizes being acumulted and how they are being reported on upload.

Test Node.js
  console.log node_modules/interface-ipfs-core/src/files.js:213
    file size is: 4540

  console.log node_modules/interface-ipfs-core/src/files.js:213
    file size is: 581878

  console.log node_modules/interface-ipfs-core/src/files.js:213
    file size is: 2294

  console.log node_modules/interface-ipfs-core/src/files.js:213
    file size is: 11685

  console.log node_modules/interface-ipfs-core/src/files.js:213
    file size is: 0

  console.log node_modules/interface-ipfs-core/src/files.js:213
    file size is: 6

  console.log node_modules/interface-ipfs-core/src/files.js:213
    file size is: 5

  console.log node_modules/interface-ipfs-core/src/files.js:213
    file size is: 0

  console.log node_modules/interface-ipfs-core/src/files.js:217
    beginning upload:

  console.log node_modules/interface-ipfs-core/src/files.js:218
    ================================================

  console.log node_modules/interface-ipfs-core/src/files.js:223
    Progress is: 4540

  console.log node_modules/interface-ipfs-core/src/files.js:223
    Progress is: 262144

  console.log node_modules/interface-ipfs-core/src/files.js:223
    Progress is: 524288

  console.log node_modules/interface-ipfs-core/src/files.js:223
    Progress is: 581878

  console.log node_modules/interface-ipfs-core/src/files.js:223
    Progress is: 2294

  console.log node_modules/interface-ipfs-core/src/files.js:223
    Progress is: 11685

  console.log node_modules/interface-ipfs-core/src/files.js:223
    Progress is: 6

  console.log node_modules/interface-ipfs-core/src/files.js:223
    Progress is: 5

daviddias · 2017-10-18T15:04:47Z

test/request-api.spec.js

+      const ipfs = ipfsAPI('/ip4/127.0.0.1/tcp/6001')
+      /* eslint-disable */
+      ipfs.files.add(Buffer.from('Hello there!'), (err, res) => {
+        // TODO: error's are not being correctly


@dryajov track this in an issue.

bmordan and others added 2 commits October 1, 2017 18:50

Adds wiring for a progress bar

2b3a577

Passes the progress bar option for tranfers over http. Progress is shown for your payload being streamed upto the server. There is a pause. Then you get your list of files.

feat: Support specify hash algorithm in files.add (#597)

c749c36

* support support specify hash alg * Add test for add with --hash option. Pass raw cid/multihash to ipfs object get/data * Allow object get/data to accept CID * Var naming tweaks from review

dryajov changed the title ~~Ipfs api progress~~ Ipfs file upload progress Oct 2, 2017

dryajov force-pushed the ipfs-api-progress branch from 789d8aa to 4eb42f4 Compare October 2, 2017 01:01

dryajov force-pushed the ipfs-api-progress branch from 4eb42f4 to 78c6d0f Compare October 2, 2017 01:08

feat: track progress events

10b0bde

dryajov force-pushed the ipfs-api-progress branch from 78c6d0f to 10b0bde Compare October 2, 2017 01:24

lidel reviewed Oct 2, 2017

View reviewed changes

victorb reviewed Oct 5, 2017

View reviewed changes

daviddias mentioned this pull request Oct 6, 2017

Adds wiring for a progress bar #596

Closed

daviddias self-assigned this Oct 6, 2017

daviddias suggested changes Oct 6, 2017

View reviewed changes

tesT: add more tests

8885ddb

victorb reviewed Oct 6, 2017

View reviewed changes

daviddias assigned dryajov and unassigned daviddias Oct 6, 2017

fix: remove .only from tests

843e699

daviddias added the ready label Oct 13, 2017

feat: signal error on reponse stream if x-stream-error

1845f2b

ghost added the in progress label Oct 14, 2017

test: adding tests for trailer header errors

1891bbf

dryajov force-pushed the ipfs-api-progress branch from c01db10 to 1891bbf Compare October 14, 2017 21:28

dryajov changed the title ~~Ipfs file upload progress~~ Awesome progress bar functionality Oct 15, 2017

dryajov changed the title ~~Awesome progress bar functionality~~ Awesome progress bar Oct 15, 2017

dryajov changed the title ~~Awesome progress bar~~ Ipfs file upload progress Oct 15, 2017

dryajov mentioned this pull request Oct 15, 2017

Awesome progress bar endeavor #608

Closed

5 tasks

daviddias added P0 - Critical and removed in progress ready labels Oct 17, 2017

chore: update interface-ipfs-core

ecbf3e6

ghost assigned daviddias Oct 18, 2017

ghost added the in progress label Oct 18, 2017

Merge branch 'master' into ipfs-api-progress

dca5925

chore: upgrade to latest interface-ipfs-core

9a11a83

daviddias approved these changes Oct 18, 2017

View reviewed changes

daviddias reviewed Oct 18, 2017

View reviewed changes

daviddias approved these changes Oct 18, 2017

View reviewed changes

daviddias merged commit e2d894c into master Oct 18, 2017

ghost removed the in progress label Oct 18, 2017

daviddias deleted the ipfs-api-progress branch October 18, 2017 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ipfs file upload progress #604

Ipfs file upload progress #604

dryajov commented Oct 2, 2017

dryajov commented Oct 2, 2017 •

edited

Loading

codecov bot commented Oct 2, 2017 •

edited

Loading

dryajov commented Oct 2, 2017

lidel Oct 2, 2017

dryajov Oct 2, 2017

lidel Oct 2, 2017

victorb Oct 5, 2017

daviddias Oct 6, 2017

daviddias commented Oct 6, 2017

daviddias Oct 6, 2017

victorb Oct 6, 2017

daviddias commented Oct 6, 2017 •

edited by dryajov

Loading

dryajov commented Oct 6, 2017

dryajov commented Oct 6, 2017

dryajov commented Oct 6, 2017 •

edited

Loading

dryajov commented Oct 6, 2017

dryajov commented Oct 6, 2017

dryajov commented Oct 6, 2017

daviddias commented Oct 9, 2017

dryajov commented Oct 9, 2017 •

edited

Loading

daviddias commented Oct 9, 2017

daviddias commented Oct 12, 2017

dryajov commented Oct 12, 2017 •

edited

Loading

dryajov commented Oct 15, 2017

dryajov commented Oct 15, 2017 •

edited

Loading

daviddias commented Oct 18, 2017

dryajov commented Oct 18, 2017 •

edited

Loading

daviddias Oct 18, 2017

Ipfs file upload progress #604

Ipfs file upload progress #604

Conversation

dryajov commented Oct 2, 2017

dryajov commented Oct 2, 2017 • edited Loading

codecov bot commented Oct 2, 2017 • edited Loading

Codecov Report

dryajov commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Oct 6, 2017 • edited by dryajov Loading

dryajov commented Oct 6, 2017

dryajov commented Oct 6, 2017

dryajov commented Oct 6, 2017 • edited Loading

dryajov commented Oct 6, 2017

dryajov commented Oct 6, 2017

dryajov commented Oct 6, 2017

daviddias commented Oct 9, 2017

dryajov commented Oct 9, 2017 • edited Loading

daviddias commented Oct 9, 2017

daviddias commented Oct 12, 2017

dryajov commented Oct 12, 2017 • edited Loading

dryajov commented Oct 15, 2017

dryajov commented Oct 15, 2017 • edited Loading

daviddias commented Oct 18, 2017

dryajov commented Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

dryajov commented Oct 2, 2017 •

edited

Loading

codecov bot commented Oct 2, 2017 •

edited

Loading

daviddias commented Oct 6, 2017 •

edited by dryajov

Loading

dryajov commented Oct 6, 2017 •

edited

Loading

dryajov commented Oct 9, 2017 •

edited

Loading

dryajov commented Oct 12, 2017 •

edited

Loading

dryajov commented Oct 15, 2017 •

edited

Loading

dryajov commented Oct 18, 2017 •

edited

Loading