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

Move to minimalkv #462

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xhochy
Copy link
Contributor

@xhochy xhochy commented Apr 21, 2021

Description:

Since simplekv is unmaintained and we cannot add new maintainers, this PR moves to its successor project minimalkv

  • Closes #xxxx
  • Changelog entry

@fjetter
Copy link
Collaborator

fjetter commented Apr 23, 2021

For the sake of compatibility, I'm wondering if simplekv stores are still compatible or not. and how/if we can ensure this and manage the deprecation properly

@xhochy
Copy link
Contributor Author

xhochy commented Apr 23, 2021

For the sake of compatibility, I'm wondering if simplekv stores are still compatible or not. and how/if we can ensure this and manage the deprecation properly

We didn't change the API in any aspect, the move was solely a rename of the top-level namespace. Passing in an existing simplekv store or a storefact-URL will work without issues. Having simplekv / storefact installed alongside minimalkv also works.

@xhochy
Copy link
Contributor Author

xhochy commented Apr 23, 2021

Verified locally that data-engineering-collective/minimalkv#24 fixes the documentation build issues. I'll retrigger the PR here once that has been released.

@xhochy xhochy force-pushed the move-to-minimalkv branch 2 times, most recently from 9a0774d to 12aee3a Compare April 26, 2021 07:50
@xhochy
Copy link
Contributor Author

xhochy commented Apr 26, 2021

All tests passed except the one that is also failing on master. This is probably an issue with the latest pandas nightly and unrelated to the simplekv -> minimalkv switch.

@xhochy
Copy link
Contributor Author

xhochy commented Apr 27, 2021

Figured out the issue we see in the tests confirming that it is totally unrelated to this PR:dask/dask#7610

OK to merge?

@stephan-hesselmann-by
Copy link
Collaborator

Thank you @xhochy for the contribution and for looking into the broken nightly test. We are discussing a possible switch to minimalkv internally, will get back to you.

@xhochy
Copy link
Contributor Author

xhochy commented May 3, 2021

@stephan-hesselmann-by Any update here or any information you would need? I did contact BY before starting minimalkv but as I'm no longer part of it, I'm not sure I reached out to all relevant parties. Switching should work quite easily as this is designed with care so that you can mix & match simplekv, minimalkv and storefact in one environment easily until the whole stack is migrated to minimalkv.

@fjetter
Copy link
Collaborator

fjetter commented May 3, 2021

If it helps with migration efforts we could think about implementing a proxy in kartothek (yet anotherget_store) which would point to the "current" backend store lib. FWIW, I don't think this is really necessary as long as minimalkv doesn't change the class interfaces without announcement

@stephan-hesselmann-by
Copy link
Collaborator

Unfortunately I don't have a definitive answer yet. The issue for us is not only Kartothek but also other Blue Yonder artifacts that currently use simplekv. That is also the reason for the indecisiveness.

@xhochy if we do decide to migrate to minimalkv, would you be willing to grant Blue Yonder "co-ownerhsip"? We would want to avoid another simplekv situation in the future and also make sure that the interface stays compatible with simplekv as long as it is used by Blue Yonder.

@xhochy
Copy link
Contributor Author

xhochy commented May 3, 2021

Unfortunately I don't have a definitive answer yet. The issue for us is not only Kartothek but also other Blue Yonder artifacts that currently use simplekv. That is also the reason for the indecisiveness.

They can use simplekv as a direct import and as a dependency, no need to change anything there. We already use minimalkv together with Kartothek and have simplekv and storefact installed.

@xhochy if we do decide to migrate to minimalkv, would you be willing to grant Blue Yonder "co-ownerhsip"? We would want to avoid another simplekv situation in the future and also make sure that the interface stays compatible with simplekv as long as it is used by Blue Yonder.

Happy to give access to people who contribute (see http://theapacheway.com/merit/), I don't want to hand over control to Blue Yonder just because they use it*. What would be your benefit / intention here exactly?

*You neither have full access to pandas either?, or more concrete: I would also like to have access back to kartothek because I use and advocate for it (also created it) but at the time of writing, I don't have that, putting me in the same situation. Thus for me this kind of argument is quite weird.

@xhochy
Copy link
Contributor Author

xhochy commented May 3, 2021

Note I gave @fjetter access to minimalkv data-engineering-collective/minimalkv#38 as he helped in the past and current time with it a bit. It may not be so relevant for you but that adds a QuantCo-independent maintainer to it.

@fjetter
Copy link
Collaborator

fjetter commented May 5, 2021

simplekv is de-factor not maintained and ownership is questionable. As we've seen with simplekv, both ownership and question does also not necessarily connect with admin or write access.
minimalkv is maintained and ownership is clearly defined. This PR removes therefore a high risk component which caused issues in the past with a better alternative.

The issue for us is not only Kartothek but also other Blue Yonder artifacts that currently use simplekv. That is also the reason for the indecisiveness.

Whether or not BY decides to "migrate" from simpelkv to minimalkv is irrelevant for this PR (although I can highly recommend). If you decide to use both, you are free to do so. The question for this PR would be more appropriately whether this change is breaking for the library kartothek. As it stands right now, you can use both libraries but by default we'll install minimalkv with this PR.

@fjetter
Copy link
Collaborator

fjetter commented May 5, 2021

#376 implemented duck typing for the storage interface. as long as this stays, the choice for simplekv or minimalkv should not impact functionality

@stephan-hesselmann-by
Copy link
Collaborator

I agree with your points and also share some your frustrations with this situation. I will try to argue in favor of this PR but as you know I cannot guarantee success.

@mlondschien
Copy link
Contributor

Are there any updates on this?

@felix-marczinowski-by
Copy link

The original Maintainer of simplekv has agreed to add additional maintainers/hand over maintainership.
@xhochy is one of the designated new maintainers, which should resolve the situation that lead to the fork.

I hope that once the formal changes are through, we can go back to just simplekv, this time with a bit more lively development!

@xhochy
Copy link
Contributor Author

xhochy commented May 19, 2021

I don't think that adding maintainers to the existing mbr/simplekv repository fixes the root of the issue. We will still not be able to fully manage that without the help of mbr.

Also, contrary to any mails, I have not yet received any rights over there.

@felix-marczinowski-by
Copy link

As I understood, mbr would extend/transfer ownership and not keep a special role. You would have all the rights he and the other new maintainers have.

@timostrunk timostrunk mentioned this pull request Nov 4, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants