Implement explicit imports throughout Catalyst.jl #1317
Conversation
- Convert all `using PackageX` to `using PackageX: specific_functions` - Add explicit imports for all packages in main module and extensions - Add SymbolicIndexingInterface to dependencies for direct imports - Fix CartesianGridReJ typo (should be CartesianGridRej) - Add QA test suite with ExplicitImports.jl and Aqua.jl - Format all changed files with JuliaFormatter using SciMLStyle - Ensure package follows best practices for explicit imports This change improves code clarity, reduces namespace pollution, and makes dependencies more explicit. The QA tests ensure continued compliance with explicit import standards. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Skip deps_compat test as it incorrectly flags stdlib packages - Use @test_skip for implicit imports and non-public access tests - This allows gradual migration while still tracking progress - Tests will warn about issues without failing CI
- Enable QA tests when GROUP=All (not just GROUP=QA) - Fix CairoMakie extension to properly import from Catalyst - Remove duplicate import that was causing conflicts
- Use @test_broken for known issues we're tracking - Properly handle implicit imports detection - Filter allowed stale imports for macros and extensions - Allow ModelingToolkit internal access as it's necessary - Make tests more informative with better warnings - Tests now fail on unexpected issues but allow known exceptions
|
@AayushSabharwal https://github.com/SciML/Catalyst.jl/actions/runs/16866870345/job/47774970326?pr=1317#step:6:696 let's triage this list. Go through and mark a bunch as public that should be public, but we should figure out what to do with what isn't public. i think just generally cleaning up some of the bad behavior here will make the v10 update easier so I want to do that early. |
|
Sure |
|
I opened JuliaSymbolics/Symbolics.jl#1614, #1318, SciML/ModelingToolkit.jl#3884 for this. There are also some unresolved issues in Catalyst:
|
|
The serialisation writes a Catalyst model to a .jl file, basically recreating the code which was (could have) been used to create it: https://docs.sciml.ai/Catalyst/stable/model_creation/model_file_loading_and_export/#model_file_import_export_crn_serialization
Changing which function is fetched from which package sounds good, and to be fair, what is where has changed some over the last 5 years or so. If you give us the complete list (which I think you just did) we will modify accordingly. Units seems slightly messy, probably best, as you suggest, to let it be like this for now and when MTK is fully happy with how units are handled we chang acordingly. Until then we just hope nothing breaks (things are tested, and we don't actually document units right now anyway, so should be fine). |
No, it's complete, tested, and has an interface to return all solutions. It's been like that for a few years now. I'll stick the bot on that, that's easy to resolve.
This is the major red flag portion though. I think I will just need to mark the test here as broken and we'll need to crack these one by one before attempting to finish MTK v10.
That's probably easy for the bots to figure out. So the main thing seems to be to drop the non-standard homotopy implementation and create standard serialization APIs for Catalyst to use so it can drop the internals stuff going on here. |
Not quite a year yet 😅
MTK can do this for |
Yeah, at some point we should do. There was also some comment about it in the Slack at some point. Generally I think the Catalyst bit actually works quite well (and have to bonus bit that it pretty much goes through all features, which have enabled us to catch some rather obscure bugs). But probably something for the future. If you have a list of internal stuff there it would be useful, and we can see what is/is not needed. |
|
No, not at some point. This is the most egregious violation of the QA tests I've seen by a few orders of magnitude 😅. The "bad" repos had 2 functions doing the wrong thing. Catalyst has like 100. This right here is exactly why the repo is so hard to maintain! That list in CI is a quantifiable indication that Catalyst alone has more hacky workaround and incorrect assumptions than the rest of SciML, JuliaDiff, and JuliaArrays combined! If we want to make this library easy to maintain, it needs to get cleaned up. It's not that we don't have time to fix this, it's that we don't have the time to maintain a library in this state. Some of this is due to MTK/Symbolics being slow to define public APIs, that's step number 1. Some of this is due to missing APIs in MTK/Symbolics, for example a serialization API that is defined and usable for here, that's step number 2. Some of this is due to avoiding public APIs and relying on internals when there is a faster/more robust/etc. public API, those moves are step number 3. Then step number 4 is to update to MTK v10, which should be pretty trivial if all internals are avoided as there's deprecation paths everywhere (we were able to bump the entire Dyad stack / compiler / VS Code tools etc. without a change to them just by using the deprecation paths, so Catalyst if done right should be the easy case!) The good news is that a good chunk of this is easy to bot. I estimate we'll get through it in about a month and we'll come out on the other side with something we can all be much happier with maintaining. |
|
I'd argue that part of the reason is that MTK have moved on quite a lot from the internals that was used when Catalyst moved to MTK, and that it has been unclear what is internal/not internal. I.e. back in the day we moved If we can get a clear list of what functions we should replace, and what the equivalent functions are, I am happy to replace them in though, that sounds all good to me. |
|
I think the list is pretty clear? |
|
Sorry, didn't realise we got a list in CI. However, some things it would be useful to get repalcements for, or options for new API. I.e. initially I had my own workflows for parsing equations, but then I explicitly changed to
I.e. I really think there are 3 levels of functions:
Case (2) probably should not be directly exported by MTK, by importable into e.g. Catalyst. I think it could be useful to make clear in MTK's API which functions that e.g. we in Catalyst can use for this purpose. |
|
Probably getting a list of (2) right now will be annoying though, and probably will happen as the MTK docs get increasingly straightened out. But let's start with the easy cases and slow work this list in Catalyst down. I will have a look as there probably are many I could have removed myself. |
|
2 shouldn't exist. The plan for these next few weeks is to completely define 1 and use Catalyst as a final boss. Either something that is internally needs to be documented and made public, or we have to give you an alternative that covers the functionality. There is no middle ground there. So some of it is things like #1318, changing Catalyst to use Public API, and some of it is like SciML/ModelingToolkit.jl#3884 changing the Public API.
No, it's not hard. I just showed you here that we now have automated tools to track down all of (2) 😅 . Catalyst is the only repo that hasn't adopted all of the checkers and CI tests for this yet, but this PR is what is adding all of that. To get it to pass though we'll need to walk the ecosystem a few times. But we have bots to then go clean them up. Give a few days for this to work out the obvious cases and we should have a list of about 10 things instead. But if you want to start, the clear (2) is the serialization and homotopy parts, I think the rest are likely simple though after getting all of the simple things done it'll show if there's anything else in the list. |
|
Ok, if the split is all 1 and 3 that is OK by me. Let's just walk the list and determine which is needed by Catalyst and what is not, there is probably a decent bunch of each. Will probably need @isaacsas to be around as well, as he knows a lot of these functions a lot better than me. |
|
We’re also going to need api to extract and detect symbolic parameters and variables from jumps/equations/brownians etc, and to then get these into defaults. Some of these are new features to have consistency with System we want in the next Catalyst breaking release, but are not used currently, so this needs to be kept in mind in determining what is “public” in MTK (basically most of the MTK System constructor analyses is likely also needed in Catalyst’s ReactionSystems constructor). |
|
Regarding dropping use of |
No those should be marked public. |
|
I tink quite a lot of the internal usage in the serialisation is that I esentially have to take a symbolic variable (e.g. X(t) [description = "A description"]right now this basically have a manual check for all metadata I know about. If we have a normal MTK function for carrying out this kind of operation, taht should cut quite a lot of stuff. |
|
Yes that just needs a clean API for you to use. |
|
Sounds good, would definitely make that part of the code a lot better and long-term reliable as well. |
Summary
using PackageXstatements to explicit importsusing PackageX: specific_functionsChanges Made
Main Module (src/Catalyst.jl)
Extensions
Quality Assurance
test/qa.jltest/runtests.jlBug Fixes
CartesianGridReJ→CartesianGridRejTest Plan
Benefits
Notes
🤖 Generated with Claude Code