-
Notifications
You must be signed in to change notification settings - Fork 99
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
Consolidate Packages into POMDPTools.jl #290
Comments
I don't know. It seems the Julian way is to have small modules that do one thing well and there can be a larger meta-package that just reexports things and contains all the documentation. However, this is the way things seem to be working in Julia because the DifferentialEquations.jl ecosystem does it and is quite popular and influential. Better documentation is definitely important in general but having separate modules doing different things is better from the perspective of maintainers usually as they can focus on their module and make sure the APIs stay consistent. However, as a single maintainer it can seem better to have a single package containing everything. So it all depends on how large we expect the JuliaPOMDP ecosystem to grow. |
I don't think this applies to POMDPToolbox because all of the things in there are small, highly-coupled tools that need to be maintained together. It makes sense to have solvers and problems and other more complex tools like particle filters as different packages because they will likely have a single maintainer, but in practice the small basic tools all need to be maintained together by the same group of people anyways. |
Maybe a compromise would be to have the |
The other problem besides the fact that users have to import a bunch of things is that it is a huge pain to maintain what we have now. It is taking me days to make upgrades instead of hours because these packages are so tightly coupled. You can't safely and easily upgrade them one-by-one because they all rely on each other for tests :( |
The thing we should have done was to write better unit tests that didn't depend on other packages. But unfortunately no one likes writing new tests 😿. Obviously, if you feel this is too much a burden, we should collect them back into POMDPToolbox or something, just like CuArrays.jl etc got combined together into CUDA.jl recently. But it will be a mess in terms of documentation and Google SEO for a while, so we need to time it right. |
I guess the coupling is stronger than just the unit tests. And I admit that finding out in which package to find which method was rather difficult in the beginning (and still is sometimes). Regarding CUDA.jl: I actually think that this is an excellent example for our problem. They moved things to a single package, merging the functionality of CuArrays.jl, CUDAnative.jl, CUDAapi.jl and CUDAdrv.jl into CUDA.jl and only factoring out the generic stuff and common interface into GPUArrays.jl. I really enjoyed this change as a user and I think it makes it also easier to contribute to their eco-system. Furthermore, they also did a good job in communicating this change IMO. Every README simply starts with something like this: And if we think this is not enough, we can still add a warning that is displayed when loading one of the deprecated packages. So migration should not be too much of an issue from a SEO perspective. |
I don't think this would actually completely fix the problem because we would still need integration tests. Yeah, I think the CUDA example is a very good example to look at. In any case, this will not be a big priority for me for a while since I have already gone through the painful part for this round. Will probably need funding and/or a student who is into this kind of thing for it to happen 😄 . |
JuliaPOMDP/POMDPPolicies.jl#36 has again convinced me that this is the right thing to do. |
The packages to be merged are
|
We should probably still keep POMDPTesting separate? Good to have a lightweight dependency that's required only for tests. |
@rejuvyesh thanks for the thought... maybe. It seems unlikely that anyone would use POMDPTesting without needing anything from the other packages though, and I don't actually thing this will be that heavy in terms of dependencies |
It's happening!! https://github.com/JuliaPOMDP/POMDPTools.jl |
I am going to start working on this tomorrow 🎉 One key decision I made is that POMDPTools should live in the POMDPs.jl github repo (see https://discourse.julialang.org/t/usage-of-subdirectories-to-store-multiple-packages-in-a-single-repo/55534 for discussion of this pattern). The main reason for this is to have unified docs for POMDPs.jl (the language) and POMDPTools.jl (the standard library). If we keep them in the same repo (still different packages of course), then updates to code and docs can be in the same PR. I also plan to export all functions at the top level, (e.g. you will be able to do If anyone has misgivings about these decisions let me know ASAP! |
One of the main annoying things about using POMDPs.jl at the moment is remembering what packages all the tools are in.
When we made the decision to split up POMDPToolbox, we did it because it was hard to discover everything in there, we thought it would be easier to maintain smaller packages, and I voted against other ideas like submodules.
In hindsight, I think documentation is a much better solution than smaller packages. Now that I am in a position where I can maintain it for a long time, I think it makes more sense to actually put it back into one package. Also I am no longer opposed to submodules.
Anyways, I think this would happen sometime in the summer, but wanted to give people a chance to chime in if they have any opinions.
The text was updated successfully, but these errors were encountered: