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

Replace Juno with ProgressLogging #1643

Closed
wants to merge 413 commits into from
Closed

Replace Juno with ProgressLogging #1643

wants to merge 413 commits into from

Conversation

DhairyaLGandhi
Copy link
Member

cc @pfitzseb @vchuravy

@pfitzseb what would be the equivalent to replace the rendering from Juno?

@pfitzseb
Copy link

You could use https://github.com/pfitzseb/TreeViews.jl instead, but I'd recommend just deleting that code, tbh.

@darsnack
Copy link
Member

darsnack commented Jun 29, 2021

The test errors shouldn't appear in practice, but the following should work for iterators that don't have axes defined.

n = length(data)
@withprogress for (i, d) in enumerate(data)
  try
    gs = gradient(ps) do
      loss(batchmemaybe(d)...)
    end
    update!(opt, ps, gs)
    cb()
  catch ex
    if ex isa StopException
      break
    elseif ex isa SkipException
      continue
    else
      rethrow(ex)
    end
  end
  @logprogress i / n
end

@DhairyaLGandhi
Copy link
Member Author

That assumes the length of the dataset is known already iiuc. Iteration does not.

@darsnack
Copy link
Member

True, but what would you log then? There are some issues about open ended logging on ProgressLogging.jl, but AFAIK the package only supports finite length logging.

@darsnack
Copy link
Member

Maybe we don't call @logprogress for unknown or infinite length iterators?

DhairyaLGandhi and others added 25 commits July 30, 2021 20:08
Updating docs

Co-authored-by: Kyle Daruwalla <[email protected]>
Updating docs

Co-authored-by: Kyle Daruwalla <[email protected]>
Updating docs

Co-authored-by: Kyle Daruwalla <[email protected]>
Updating docs

Co-authored-by: Kyle Daruwalla <[email protected]>
1675: Adding GRUv3 support. r=darsnack a=mkschleg

As per the starting discussion in #1671, we should provide support for variations on the GRU and LSTM cell. 

In this PR, I added support for the GRU found in v3 of the [original GRU paper](https://arxiv.org/abs/1406.1078). Current support in Flux is for v1 only. [Tensorflow](https://www.tensorflow.org/api_docs/python/tf/keras/layers/GRU) supports several variations, with this as one of the variations. 

While the feature is added and usable in this PR, this is only a first pass at a design and could use further iterations. Some questions I have:
- Should we have new types for each variation of these cells? (another possibility is through parametric options)
- Should we have a shared constructor similar to Tensorflow/Pytorch? (it might make sense to rename the current GRU to GRUv1 if we want to do this).

### PR Checklist

- [x] Tests are added
- [x] Entry in NEWS.md
- [x] Documentation, if applicable
- [ ] API changes require approval from a committer (different from the author, if applicable)


Co-authored-by: Matthew Schlegel <[email protected]>
Co-authored-by: Matthew Schlegel <[email protected]>
ToucheSir and others added 14 commits December 2, 2021 09:08
Temporary measure to unblock CI while we investigate the root cause. I think `@test_broken` is better than increasing the tolerance, as the latter makes it easier to forget about this one entirely.
Only add PR comment with docs build if the docs label is added
Update train.jl to add a more detailed `train!` docstring
1797: Mark destructure gradient test as broken r=DhairyaLGandhi a=ToucheSir

Temporary measure to unblock CI while we investigate the root cause. I think ``@test_broken`` is better than increasing the tolerance, as the latter makes it easier to forget about this one entirely.


Co-authored-by: Brian Chen <[email protected]>
1805: Add buildkite step to run on Julia LTS r=DhairyaLGandhi a=DhairyaLGandhi

Allow bors to unblock some PRs as well.

Co-authored-by: Dhairya Gandhi <[email protected]>
@ToucheSir
Copy link
Member

If you'd like, I can do the cleanup work for this.

@DhairyaLGandhi
Copy link
Member Author

This would need a bit more work than a rebase. ProgressLogging isn't exactly compatible with Juno's, but as long as we have some fallbacks in place, we can merge this.

@ToucheSir
Copy link
Member

The only API we're switching over is Juno.@progress to ProgressLogging.@progress. For the 2 places the former is used, the latter should have the same behaviour.

@DhairyaLGandhi
Copy link
Member Author

superseded by #1807

@DhairyaLGandhi
Copy link
Member Author

There's no guarantee for a known length to the data arg in train!. In some cases, the termination will happen only once some accuracy criteria is met and more samples would be generated indefinitely

@DhairyaLGandhi DhairyaLGandhi deleted the dg/progress branch December 11, 2021 01:32
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.