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

Split out most functionality into ControlSystemsBase #746

Merged
merged 4 commits into from
Sep 8, 2022
Merged

Conversation

baggepinnen
Copy link
Member

@baggepinnen baggepinnen commented Sep 8, 2022

Ref: #738

Timings without any system image:

julia> @time using ControlSystemsBase
  1.70 seconds
julia> @time using ControlSystems
  21.71 seconds

The difference in pre-compilation time is several minutes.

This moves most functionality into the sub-folder lib/ControlSystemsBase, which I intend to release in General as a separate package. The documentation is still contained in the same location as before, and will hopefully work and look more or less the same. I edited some of the examples in the docs to only load ControlSystemsBase if applicable to indicate the readers that this is a possibility, I also explained the differences in the readme and in the installation section in the docs.

All tests fail since ControlSystemsBase is not registered, so I think we'll just have to

  1. merge
  2. register ControlSystemsBase
  3. make sure tests pass (tests for both packages pass locally)
  4. Tag new release of ControlSystems

@albheim does this sound reasonable?

@JuliaControlBot
Copy link

Something failed when generating plots. See the log at https://github.com/JuliaControl/ControlExamplePlots.jl/actions/runs/3014615539?check_suite_focus=true for more details.

@JuliaControlBot
Copy link

Something failed when generating plots. See the log at https://github.com/JuliaControl/ControlExamplePlots.jl/actions/runs/3014807745?check_suite_focus=true for more details.

@JuliaControlBot
Copy link

Something failed when generating plots. See the log at https://github.com/JuliaControl/ControlExamplePlots.jl/actions/runs/3014806277?check_suite_focus=true for more details.

@albheim
Copy link
Member

albheim commented Sep 8, 2022

That is quite a difference in load time, nice job!

One thing I'm thinking about is if the tests should not use the local version of ControlSystemsBase (dev it) and thus not need it registered (we should still register though). If not changes in base would require a long chain before actually being tested?
push changes A -> register changes A -> push other changes B that now pull A -> A gets tested

@baggepinnen
Copy link
Member Author

Good point, I'm all for speeding up developer workflows! :)

We need to find a way to guarantee that we indeed register a new version of Base as well if something has changed there. If we test ControlSystems with dev Base, we can pass tests that would fail for the user since no new release of Base has been made.
We could potentially run tests with both dev and the latest registered version, but that will lead to very long CI times. In practice, maybe this is not a huge problem since the amount of functionality that lives in ControlSystems after this PR is rather small, and will likely not be touched nearly as often as the stuff in Base.

@baggepinnen
Copy link
Member Author

baggepinnen commented Sep 8, 2022

I looked at how testing is done in Optimization.jl, they use the group attribute of github actions to run multiple tests in the same workflow. For each group, the top-level runtests.jl file is run. In this file, the actual tests to run are determined based on the string value of the group attribute, which is available from the global dict ENV. The changes were made here
18ca9b5

@baggepinnen
Copy link
Member Author

It appears impossible to get past

ERROR: expected package `ControlSystemsBase` to be registered

without registering the package first. I made sure ControlSystemsBase is deved before testing CS, we'll just have to be careful so that a new version of CSBase is tagged if a new version of CS depends on it. In practice, I mostly release immediately whenever something is fixed anyways so that should be okay.

@albheim
Copy link
Member

albheim commented Sep 8, 2022

We need to find a way to guarantee that we indeed register a new version of Base as well if something has changed there. If we test ControlSystems with dev Base, we can pass tests that would fail for the user since no new release of Base has been made.

True, that is not optimal either.

For each group, the top-level runtests.jl file is run.

So from what I saw I guess the group ControlSystems will run ControlSystems test with old version of ControlSystemsBase, and the group ControlSystemsBase will run ControlSystemsBase tests with dev'd version of ControlSystemsBase?

I agree that too much testing is not good either, but I feel like it is not clear to me where to draw the line. I could also think that testing ControlSystemsBase without ControlSystems might be interesting, and running ControlSystems tests with dev'd version of ControlSystemsBase could also be interesting. Though I haven't looked into how split the tests are, but it feels like the might very well affect each other.

The downside of splitting up the package, having to think of internal compatability...

One could also try to put pretty strict compat bounds so that the latest ControlSystems version only ever allow the latest ControlSystemsBase version, thus not having to test across versions. But that might also be a bit messy to keep up.

@baggepinnen
Copy link
Member Author

It's currently set up so that any change to this github repo (including all packages) triggers all tests.

  • The tests for ControlSystemsBase are independent of ControlSystems, but any change will test ControlSystems as well
  • The tests for ControlSystems use the latest state of ControlSystemsBase (dev)

They do run in parallel, so we might get some speedup from this compared to before.

So for changes to ControlSystems, we only need to tag a new version of ControlSystems. For changes to ControlSystemsBase, we will notice if the ControlSystems tests fail and will thus have to update that as well.

I think this should work out well :)

@baggepinnen baggepinnen merged commit 8cbe21a into master Sep 8, 2022
@baggepinnen baggepinnen deleted the CSBase branch September 8, 2022 13:30
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.

None yet

3 participants