extend dataloader #1152

CarloLucibello · 2020-04-29T08:21:12Z

cfr discussion in #1149. Currently DataLoader interface supports

for x in DataLoader(X)
for (x, y) in DataLoader(X, Y)

This PR adds

for (x,) in DataLoader((X,))
for (x, y) in DataLoader((X, Y))

Edit:
the constructor in 2. is removed in this PR

CarloLucibello · 2020-04-29T10:43:22Z

Removed multi-arg constructor as discussed in #1149

CarloLucibello · 2020-04-29T10:56:48Z

fix #1088

DhairyaLGandhi · 2020-04-29T15:57:24Z

There's a few empty lines added here, good to check them

DhairyaLGandhi · 2020-05-03T11:46:49Z

src/data/dataloader.jl

-    DataLoader(data, batchsize, nx, partial, imax, [1:nx;], shuffle)
+    imax = partial ? n : n - batchsize + 1
+    ids = 1:min(n, batchsize)
+    DataLoader(data, batchsize, n, partial, imax, [1:n;], shuffle)


Do we end up creating a copy of the data that we pass into it? It will be annoying while using GPUs where VRAM is already at a premium.

Maybe it should specialize on if we can move data to the GPU in a separate thread while the training is running and add it to a buffer which can be done for like a couple batches worth, so we can eliminate time to move, retrieve data without saturating the GPU?

Do we end up creating a copy of the data that we pass into it? It will be annoying while using GPUs where VRAM is already at a premium.

the entire datasets is kept with no copying. We have copying only when indexing to produce a mini-batch

Maybe it should specialize on if we can move data to the GPU in a separate thread while the training is running and add it to a buffer which can be done for like a couple batches worth, so we can eliminate time to move, retrieve data without saturating the GPU?

this should be done at some point

src/data/dataloader.jl

DhairyaLGandhi · 2020-05-04T06:31:13Z

There was mention on slack about some type instability, could we clear that

cossio · 2020-05-04T09:49:59Z

See #1159

CarloLucibello · 2020-05-04T10:29:48Z

fix #1159

CarloLucibello · 2020-05-04T13:09:36Z

other changes

bumped pkg version
removed the Manifest (as the discussion in CI for julia 1.0 Zygote.jl#629 applies also to Flux, see also Remove Manifest from master #1164 )
implement also the train part of Cleaner training loop #1149 (comment) (since this PR was already breaking, let's do all of it and bump to v0.11)

DhairyaLGandhi · 2020-05-04T13:07:42Z

src/optimise/train.jl

-      else
-        gs = gradient(ps) do
-          loss(d...)
-        end


Already in #1149 ,let's not repeat it here

DhairyaLGandhi · 2020-05-04T13:09:09Z

.travis.yml

@@ -16,7 +16,7 @@ notifications:
 jobs:
  include:
    - stage: "Documentation"
-      julia: 1
+      julia: 1.4


I think it's fixed on master

DhairyaLGandhi · 2020-05-04T13:10:16Z

src/optimise/train.jl

@@ -56,14 +56,17 @@ function stop()
  throw(StopException())
 end

+maketuple(x) = (x,)
+maketuple(x::Tuple) = x


We'd decided to not do this, so let's not

this is the last point of #1149 (comment)

DhairyaLGandhi · 2020-05-08T18:45:07Z

I'd suggest keeping the manifest changes separate as there is a separate PR and discussion unrelated to Data loader here

MikeInnes · 2020-05-12T14:22:39Z

Best to keep each PR to one logical change. For this that should be the data loaders; the training loop can happen in #1149 and we can follow up with the manifest change. As much as anything, the manifest change is going to lead to more merge conflicts.

DhairyaLGandhi · 2020-06-05T12:48:25Z

bump

cossio · 2020-06-06T15:21:07Z

src/data/dataloader.jl

+    return n
+end
+
+function _getobs(data::A, i) where A<:AbstractArray{T,N} where {T,N}


Suggested change

function _getobs(data::A, i) where A<:AbstractArray{T,N} where {T,N}

function _getobs(data::AbstractArray{T,N}, i) where {T,N}

cossio · 2020-06-06T15:24:40Z

src/data/dataloader.jl

+    getindex(data, ntuple(i->Colon(), N-1)..., i)
+end
+
+_getobs(data::Tuple, i) = ((_getobs(x, i) for x in data)...,)


Suggested change

_getobs(data::Tuple, i) = ((_getobs(x, i) for x in data)...,)

_getobs(data::Tuple, i) = map(x -> _getobs(x, i), data)

cossio · 2020-06-06T15:30:23Z

src/data/dataloader.jl

-    ids = 1:min(nx, batchsize)
-    DataLoader(data, batchsize, nx, partial, imax, [1:nx;], shuffle)
+    imax = partial ? n : n - batchsize + 1
+    ids = 1:min(n, batchsize)


Remove this line?

CarloLucibello · 2020-06-06T16:05:20Z

reinstated the Manifest and removed the changes to train! (that should happen in #1149 instead). This should be ready to go

MikeInnes · 2020-06-08T14:01:13Z

Manifest.toml

@@ -384,4 +384,4 @@ version = "0.4.20"
 deps = ["MacroTools"]
 git-tree-sha1 = "b3b4882cc9accf6731a08cc39543fbc6b669dca8"
 uuid = "700de1a5-db45-46bc-99cf-38207098b444"
-version = "0.2.0"
+version = "0.2.0"


Trailing newline is missing here

MikeInnes · 2020-06-08T14:01:52Z

src/optimise/train.jl

@@ -121,4 +121,4 @@ macro epochs(n, ex)
      @info "Epoch $i"
      $(esc(ex))
    end)
-end
+end


MikeInnes

Would like a quick cleanup of the newlines (otherwise it just adds a little noise to PRs when people have editor settings that automatically fix this stuff) but otherwise happy with the API changes.

CarloLucibello · 2020-06-08T18:00:45Z

bors r+

bors · 2020-06-08T18:29:42Z

Build succeeded:

ci/gitlab/gitlab.com

CarloLucibello added the breaking label Apr 29, 2020

CarloLucibello mentioned this pull request Apr 29, 2020

Cleaner training loop #1149

Merged

CarloLucibello force-pushed the cl/data branch 2 times, most recently from af8cf73 to 041cc91 Compare April 29, 2020 15:19

MikeInnes requested a review from DhairyaLGandhi May 1, 2020 13:15

DhairyaLGandhi reviewed May 4, 2020

View reviewed changes

CarloLucibello force-pushed the cl/data branch from d38143e to f8c8bb4 Compare May 4, 2020 18:30

CarloLucibello added this to the v0.11 milestone May 5, 2020

cossio reviewed Jun 6, 2020

View reviewed changes

CarloLucibello force-pushed the cl/data branch from f8c8bb4 to 19ee886 Compare June 6, 2020 15:56

extend dataloader

a643cb6

CarloLucibello force-pushed the cl/data branch from 19ee886 to a643cb6 Compare June 6, 2020 16:03

add news

b1f226e

MikeInnes reviewed Jun 8, 2020

View reviewed changes

src/optimise/train.jl Outdated

@@ -121,4 +121,4 @@ macro epochs(n, ex)

@info "Epoch $i"

$(esc(ex))

end)

end

end

Copy link

Member

MikeInnes Jun 8, 2020

Choose a reason for hiding this comment

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

also here

MikeInnes approved these changes Jun 8, 2020

View reviewed changes

cleanup

0cf4643

bors bot merged commit a7bbd3d into master Jun 8, 2020

cossio mentioned this pull request Jun 14, 2020

DataLoader type instability #1159

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extend dataloader #1152

extend dataloader #1152

CarloLucibello commented Apr 29, 2020 •

edited

Loading

CarloLucibello commented Apr 29, 2020

CarloLucibello commented Apr 29, 2020

DhairyaLGandhi commented Apr 29, 2020

DhairyaLGandhi May 3, 2020

DhairyaLGandhi May 3, 2020

CarloLucibello May 4, 2020

CarloLucibello May 4, 2020

DhairyaLGandhi commented May 4, 2020

cossio commented May 4, 2020

CarloLucibello commented May 4, 2020

CarloLucibello commented May 4, 2020 •

edited

Loading

DhairyaLGandhi May 4, 2020

DhairyaLGandhi May 4, 2020

DhairyaLGandhi May 4, 2020

CarloLucibello May 4, 2020

DhairyaLGandhi commented May 8, 2020

MikeInnes commented May 12, 2020

DhairyaLGandhi commented Jun 5, 2020

cossio Jun 6, 2020

cossio Jun 6, 2020

cossio Jun 6, 2020

CarloLucibello commented Jun 6, 2020

MikeInnes Jun 8, 2020

MikeInnes Jun 8, 2020

MikeInnes left a comment

CarloLucibello commented Jun 8, 2020

bors bot commented Jun 8, 2020

	function _getobs(data::A, i) where A<:AbstractArray{T,N} where {T,N}
	function _getobs(data::AbstractArray{T,N}, i) where {T,N}

	_getobs(data::Tuple, i) = ((_getobs(x, i) for x in data)...,)
	_getobs(data::Tuple, i) = map(x -> _getobs(x, i), data)

extend dataloader #1152

extend dataloader #1152

Conversation

CarloLucibello commented Apr 29, 2020 • edited Loading

CarloLucibello commented Apr 29, 2020

CarloLucibello commented Apr 29, 2020

DhairyaLGandhi commented Apr 29, 2020

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

DhairyaLGandhi commented May 4, 2020

cossio commented May 4, 2020

CarloLucibello commented May 4, 2020

CarloLucibello commented May 4, 2020 • edited Loading

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

DhairyaLGandhi commented May 8, 2020

MikeInnes commented May 12, 2020

DhairyaLGandhi commented Jun 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarloLucibello commented Jun 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeInnes left a comment

Choose a reason for hiding this comment

CarloLucibello commented Jun 8, 2020

bors bot commented Jun 8, 2020

CarloLucibello commented Apr 29, 2020 •

edited

Loading

CarloLucibello commented May 4, 2020 •

edited

Loading