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

Deprecate exists to haskey and names to keys and bump minor version in prep for HDF5 release #156

Merged
merged 3 commits into from
Dec 31, 2020

Conversation

musm
Copy link
Member

@musm musm commented Dec 29, 2020

No description provided.

@musm musm requested a review from jmert December 29, 2020 18:54
@jmert
Copy link
Contributor

jmert commented Dec 30, 2020

I think we should also do names -> keys at the same time — I've pushed a commit to do that.

Copy link
Contributor

@jmert jmert left a comment

Choose a reason for hiding this comment

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

With names -> keys, I think we're good to go.

Edit: Wait — we might need to keep the import HDF5.exists for now to have the deprecated binding be common among MAT and JLD. I'm going to try to test that.\

Edit 2: Yes, with current HDF5, latest JLD, and this branch after pushing my commit:

julia> using JLD, MAT

julia> mfile = matopen("MAT/test/v7/struct.mat")
MAT.MAT_v5.Matlabv5File(IOStream(<file MAT/test/v7/struct.mat>), false, #undef)

julia> exists(mfile, "s")
ERROR: MethodError: no method matching exists(::MAT.MAT_v5.Matlabv5File, ::String)
Closest candidates are:
  exists(::Union{HDF5.Attributes, HDF5.Dataset, HDF5.Datatype, HDF5.File, HDF5.Group}, ::AbstractString) at deprecated.jl:70
  exists(::Union{JLD.JLD00.JldDataset, JLD.JLD00.JldFile, JLD.JLD00.JldGroup}, ::String) at /home/justin/.julia/dev/JLD/src/JLD00.jl:1147
  exists(::Union{JLD.JldDataset, JLD.JldFile, JLD.JldGroup}, ::String) at /home/justin/.julia/dev/JLD/src/JLD.jl:1324
Stacktrace:
 [1] top-level scope
   @ REPL[8]:1

so it looks like we'll need to deprecate upon the imported deprecated binding, and then coordinate releases of HDF5, JLD, and MAT that all simultaneously remove the deprecation. Unfortunately, that means either a very short deprecation period for JLD and MAT, or we wait a bit on the HDF5 side.

@musm
Copy link
Member Author

musm commented Dec 30, 2020

I checkout out this MAT branch and the JLD branch with the depwarn removal.

(@v1.7) pkg> activate --shared dev
  Activating environment at `C:\Users\Mus\.julia\environments\dev\Project.toml`

julia> using MAT,JLD
[ Info: Precompiling JLD [4138dd39-2aa7-5051-a626-17a0bb65d9c8]

julia> mfile = matopen("../MAT/test\\v7\\struct.mat")
MAT.MAT_v5.Matlabv5File(IOStream(<file ../MAT/test\v7\struct.mat>), false, #undef)

julia> exists(mfile,"s")
WARNING: JLD.exists is deprecated, use haskey instead.
  likely near REPL[5]:1

I think the only thing we need to ensure is that we tag JLD and MAT at the same time with the removal, unless I'm missing something.

@musm
Copy link
Member Author

musm commented Dec 30, 2020

Oh I see, they would both export their own exists binding, so the error message would be wrong, but the method call should be correct as far as I can tell. So is that the issue?

And this would only be a problem for users importing both the MAT, JLD packages at the same time.

@jmert
Copy link
Contributor

jmert commented Dec 30, 2020

I think that accidentally works — note that it says JLD.exists is deprecated, not MAT.exists. Since the @deprecate_binding adds untyped methods, though, any exported binding is sufficient to forward to the common Base.haskey, and then type dispatch works as expected.

If both MAT and JLD are upgraded simultaneously, then there's no functional problem (though still a confusing error message), but if you get a mismatch of MAT v0.10 (next release) and JLD v0.11 (current latest), then the deprecations break, I think.

@jmert
Copy link
Contributor

jmert commented Dec 30, 2020

And this would only be a problem for users importing both the MAT, JLD packages at the same time.

Yeah, we're both replying about the same time but reaching the same conclusion — I think the key is to maintain compatibility across any valid combination of package versions, which could include with and without deprecations in MAT and JLD (or vice versa), respectively.

(Though both package restricting themselves to HDF5 v0.15 on a release would serve to synchronize package compatibility "eras". The need for a deprecation period makes that probably undesirable for the next release.)

@musm
Copy link
Member Author

musm commented Dec 30, 2020

Ok so if I understand things correctly.

And correct me if this is wrong, but our options are:

option 1:

  1. Modify this and the JLD PR to bump the HDF5 version to "v0.15" in Project.toml
  2. Bump HDF5 to "v0.15"
  3. Immediately, merge the MAT and JLD PRs to prevent an upgrade whereby older MAT and JLD version pull in HDF5 v0.15, where HDF5.exist has been excised.

option 2:

  1. Merge this PR as is.
  2. Don't remove HDF5.exists in the "v0.15" release.
  3. In the next MAT,JLD minor version bump remove the deprecations and restrict HDF5 to v0.15.
  4. Remove HDF5.exists in the v0.16 cycle of HDF5.

If this is correct, my preference is for option 1, as it seems to require fewer steps where one could screw up in the process or forget by the team someone gets to all the stages along the timeline. Open to either option, regardless.

@jmert
Copy link
Contributor

jmert commented Dec 30, 2020

Ok so if I understand things correctly.

And correct me if this is wrong, but our options are:

option 1:

  1. Modify this and the JLD PR to bump the HDF5 version to "v0.15" in Project.toml
  2. Bump HDF5 to "v0.15"
  3. Immediately, merge the MAT and JLD PRs to prevent an upgrade whereby older MAT and JLD version pull in HDF5 v0.15, where HDF5.exist has been excised.

The compat bounds in Project.toml should prevent this from happening, shouldn't it?

option 2:

  1. Merge this PR as is.
  2. Don't remove HDF5.exists in the "v0.15" release.
  3. In the next MAT,JLD minor version bump remove the deprecations and restrict HDF5 to v0.15.
  4. Remove HDF5.exists in the v0.16 cycle of HDF5.

If this is correct, my preference is for option 1, as it seems to require fewer steps where one could screw up in the process or forget by the team someone gets to all the stages along the timeline. Open to either option, regardless.

Option 3:

  1. Merge this PR (and similar for JLD).
  2. Release new major versions of MAT and JLD with deprecations (still using HDF5 v0.14).
  3. Release HDF5 v0.15 with HDF5.exists (and other deprecations) removed.
  4. Remove the deprecations from MAT and JLD, and bump requirement to HDF5 v0.15
  5. Release another major version bump for MAT and JLD, each.

@musm
Copy link
Member Author

musm commented Dec 30, 2020

The compat bounds in Project.toml should prevent this from happening, shouldn't it?

Oh your right. It's the caret specifier and for packages without a major digit it's the same as tilde, so we should be good.

@musm
Copy link
Member Author

musm commented Dec 30, 2020

Do you have a preference for which option? I think Option 3 or Option 1 in order of preference.

@musm musm changed the title Deprecate exists to haskey Deprecate exists to haskey and names to keys and bump minor version in prep for HDF5 release Dec 30, 2020
@musm musm merged commit 0efc6d8 into master Dec 31, 2020
@musm musm deleted the exists branch December 31, 2020 03:06
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.

2 participants