Skip to content

Conversation

@torfjelde
Copy link
Member

@torfjelde torfjelde commented Apr 18, 2023

What the title says

Warning: this PR also removes testing of Tracker.jl. Tracker.jl is the only AD backend requiring custom rules (i.e. isn't supported by ChainRules.jl) and hence requires quite a bit of additional developer effort to maintain and test throughout the TuringLang ecosystem. It seems to me that this is no longer worth the effort given how little (if any) use Tracker.jl sees.

@torfjelde torfjelde changed the title Dump DynamicPPL.jl version Dump DynamicPPL.jl, AdvancedVI, and Bijectors version Apr 18, 2023
@coveralls
Copy link

coveralls commented Apr 18, 2023

Pull Request Test Coverage Report for Build 4735955139

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build 4604993505: 0.0%
Covered Lines: 0
Relevant Lines: 1422

💛 - Coveralls

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (3e71a76) 0.00% compared to head (1bb195c) 0.00%.

❗ Current head 1bb195c differs from pull request most recent head 966639f. Consider uploading reports for the commit 966639f to get more accurate results

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1979   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          21      21           
  Lines        1422    1424    +2     
======================================
- Misses       1422    1424    +2     
Impacted Files Coverage Δ
src/essential/ad.jl 0.00% <0.00%> (ø)
src/inference/hmc.jl 0.00% <0.00%> (ø)
src/inference/mh.jl 0.00% <ø> (ø)
src/variational/VariationalInference.jl 0.00% <ø> (ø)
src/variational/advi.jl 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@torfjelde torfjelde changed the title Dump DynamicPPL.jl, AdvancedVI, and Bijectors version Bump DynamicPPL.jl, AdvancedVI, and Bijectors version Apr 20, 2023
@torfjelde torfjelde requested a review from devmotion April 23, 2023 12:21
@yebai yebai merged commit 81ca26f into master Apr 25, 2023
@delete-merged-branch delete-merged-branch bot deleted the torfjelde/dynamicppl-bump branch April 25, 2023 05:21
@yebai
Copy link
Member

yebai commented Apr 25, 2023

Excellent PR -- thanks @torfjelde!


Turing.setrdcache(false)
for adbackend in (:forwarddiff, :tracker, :reversediff)
for adbackend in (:forwarddiff, :reversediff)
Copy link
Member

Choose a reason for hiding this comment

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

@torfjelde I just noticed this change - this seems quite dangerous because things might start breaking silently without us noticing it while still stating officially in the package (compat entry, docs etc.) that Tracker is supported.

So I think in case this is the official policy (which IMO is a bit sad because Tracker is quite stable, much simpler than ReverseDiff or Zygote, and started to receive updates more frequently again) then IMO it should be removed from the package, docs, etc., ideally before removing tests for it.

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.

5 participants