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

remove Datasets + additional deprecations #1377

Merged
merged 15 commits into from
Nov 11, 2020
Merged

remove Datasets + additional deprecations #1377

merged 15 commits into from
Nov 11, 2020

Conversation

CarloLucibello
Copy link
Member

@CarloLucibello CarloLucibello commented Oct 31, 2020

Remove Flux's datasets in favor of data providers in the julia ecosystem.

@CarloLucibello
Copy link
Member Author

@DhairyaLGandhi are you ok with this change?

@DhairyaLGandhi
Copy link
Member

Best to deprecate no?

@CarloLucibello
Copy link
Member Author

problem is each dataset is a module, I searched for a while but I didn't find how to deprecate a module

@DhairyaLGandhi
Copy link
Member

Hmm, not sure but we might end up with add depwarns to the methods that people would call for the datasets?

@CarloLucibello CarloLucibello changed the title remove Datasets remove Datasets + additional deprecations Nov 6, 2020
@CarloLucibello
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Nov 6, 2020
@bors
Copy link
Contributor

bors bot commented Nov 6, 2020

try

Build succeeded:

@DhairyaLGandhi
Copy link
Member

Many thanks to @KristofferC !!

 julia> module Foo
           module Bar
           end
           
           Base.@deprecate_binding Bazz Bar
        end
 Main.Foo
 
 julia> Foo.Bazz
 WARNING: Foo.Bazz is deprecated, use Bar instead.
   likely near REPL[2]:1
 Main.Foo.Bar

List of packages built on top of Flux:
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid having this list here for now and it should be the synced with the list in the website. And in general we should also do a once over the packages as well

Copy link
Member Author

Choose a reason for hiding this comment

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

and it should be the synced with the list in the website. And in general we should also do a once over the packages as well

I don't understand

@CarloLucibello
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 11, 2020

Build succeeded:

@bors bors bot merged commit 773d0a1 into master Nov 11, 2020
@DhairyaLGandhi
Copy link
Member

Why did this not follow the deprecations route? It makes breaking API changes without the deprecations themselves allowing users to continue using the functionality.

@CarloLucibello
Copy link
Member Author

I did introduce informative errors pointing users to MLDatasets, it should be very easy for them to switch. Also, recent examples in model-zoo have been relying on MLDatasets for a long time.
I can make a new PR with a softer deprecation path by importing MLDatasets here and rerouting its function to match the previous Flux api. I prefer not to though, it is a lot of work to do here, while users can overcome the change pretty easily

@DhairyaLGandhi
Copy link
Member

I'd prefer a warning about them being removed and using the datasets we had for now, and removing them in a couple of releases.

Agree on not adding MLDatasets as a dep

@CarloLucibello CarloLucibello added this to the v0.12 milestone Jan 5, 2021
@CarloLucibello CarloLucibello deleted the data branch January 7, 2021 08:46
bors bot added a commit that referenced this pull request Jan 8, 2021
1442: Soft deprecation for Datasets r=CarloLucibello a=CarloLucibello

Add a soft deprecations path for the datasets I brutally removed in #1377 .
I added the old tests back, verified they passed, then removed them again. 

Fix #1426


Co-authored-by: Carlo Lucibello <carlo.lucibello@gmail.com>
Co-authored-by: Dhairya Gandhi <dhairya@juliacopmuting.com>
bors bot added a commit that referenced this pull request Jan 8, 2021
1442: Soft deprecation for Datasets r=DhairyaLGandhi a=CarloLucibello

Add a soft deprecations path for the datasets I brutally removed in #1377 .
I added the old tests back, verified they passed, then removed them again. 

Fix #1426


Co-authored-by: Carlo Lucibello <carlo.lucibello@gmail.com>
Co-authored-by: Dhairya Gandhi <dhairya@juliacopmuting.com>
bors bot added a commit that referenced this pull request Jan 8, 2021
1442: Soft deprecation for Datasets r=DhairyaLGandhi a=CarloLucibello

Add a soft deprecations path for the datasets I brutally removed in #1377 .
I added the old tests back, verified they passed, then removed them again. 

Fix #1426


Co-authored-by: Carlo Lucibello <carlo.lucibello@gmail.com>
Co-authored-by: Dhairya Gandhi <dhairya@juliacopmuting.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants