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

Add test for new dependencies #3278

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Add test for new dependencies #3278

merged 1 commit into from
Sep 17, 2024

Conversation

Sbozzolo
Copy link
Member

@Sbozzolo Sbozzolo commented Sep 2, 2024

Adding new dependencies to a Julia project has downsides. First, every dependency increases the precompilation and import time of a package. For complex packages, such as CUDA, this translates in several minutes of precompilation time that every single user of ClimaAtmos (including our CI runners) always pays. Second, dependencies can increase the cost of maintaining ClimaAtmos and the surface area of things that can go wrong (a buggy update in a dependency can break ClimaAtmos). Third, dependencies increase the complexity of the project's environment, potentially causing compatibility issues and restricting the versions we can use.

This new test follows the spirit of the allocation tests and tries to capture the cost of adding a new dependency by having developers explicitly edit the file.

The only relevant commit in this PR is 68bb3f7

Everything else is #3276, where I remove some unused dependencies

@Sbozzolo Sbozzolo force-pushed the gb/test_dependencies branch 3 times, most recently from 3342882 to 68bb3f7 Compare September 3, 2024 00:38
@charleskawczynski
Copy link
Member

I'm not sure how I feel about 68bb3f7. Is this really something that we want to have a test for?

@Sbozzolo
Copy link
Member Author

Sbozzolo commented Sep 3, 2024

It is much easier to keep dependencies out than to remove them once they are in. So, I would like people to be more mindful of what they are adding and check if the new dependency is really needed.

Several people work on ClimaAtmos and currently there is no barrier to adding new dependencies. Two examples of things that I removed over the past few months that should have never gotten in: Distributions.jl (not a small library) to compute an error function, an entire image processing stack to perform a gaussian filter (through ImageFiltering). To add a tiny barrier, I follow the idea of the allocation tests, where we add a bit of friction for people to stop for a second looking at what they are doing.

@Sbozzolo Sbozzolo force-pushed the gb/test_dependencies branch 5 times, most recently from f73a70e to 50a8d31 Compare September 11, 2024 03:33
@charleskawczynski
Copy link
Member

It is much easier to keep dependencies out than to remove them once they are in. So, I would like people to be more mindful of what they are adding and check if the new dependency is really needed.

Several people work on ClimaAtmos and currently there is no barrier to adding new dependencies. Two examples of things that I removed over the past few months that should have never gotten in: Distributions.jl (not a small library) to compute an error function, an entire image processing stack to perform a gaussian filter (through ImageFiltering). To add a tiny barrier, I follow the idea of the allocation tests, where we add a bit of friction for people to stop for a second looking at what they are doing.

That's a fair point. I'm willing to try this if other people are okay with trying it. I'll request additional review

@charleskawczynski
Copy link
Member

Ah, there are more reviewers listed. @dennisYatunin, @sriharshakandala, can you please chime in?

@Sbozzolo
Copy link
Member Author

(I do agree that this should not be a test: it should be reviews that capture this sorts of problem, but we are not there yet)

@dennisYatunin
Copy link
Member

dennisYatunin commented Sep 12, 2024

I discussed this with Gabriele offline a few days ago. My thoughts are that it is ok to add something like this if unnecessary packages keep making it past review, though I think we could also get away with something a little more automated (e.g., compare the list of currently loaded packages to what's on main or in the latest release, and print a warning if the currently loaded list has anything extra).

One feature of this version that the automated version wouldn't capture, however, is that it provides a central place for people to jot down why specific packages need to be added and where they are used. Ideally, we could do this directly in Project.toml, but any comments in that file get overwritten when it is modified by the package manager.

If we want to include this as part of the design, I think the version of the test added in this PR is fine. If not, we can fix Pkg.jl so that it doesn't remove comments from Project.toml, and then we can put package-related notes in that file and turn this test into a simpler automated warning.

@Sbozzolo Sbozzolo force-pushed the gb/test_dependencies branch from 50a8d31 to 04bb1d0 Compare September 17, 2024 00:07
@Sbozzolo
Copy link
Member Author

Ok, I am merging this. We can revert it or change our approach if it causes troubles or other unintended consequences.

@Sbozzolo Sbozzolo enabled auto-merge September 17, 2024 00:08
Adding new dependencies to a Julia project has downsides. First, every
dependency increases the precompilation and import time of a package. For
complex packages, such as CUDA, this translates in several minutes of
precompilation time that every single user of ClimaAtmos (including our CI
runners) always pays. Second, dependencies can increase the cost of maintaining
ClimaAtmos and the surface area of things that can go wrong (a buggy update in a
dependency can break ClimaAtmos). Third, dependencies increase the complexity of
the project's environment, potentially causing compatibility issues and
restricting the versions we can use.

This new test follows the spirit of the regression tests and tries to capture
the cost of adding a new dependency by having developers explicitly come here
and
@Sbozzolo Sbozzolo force-pushed the gb/test_dependencies branch from 04bb1d0 to 023e355 Compare September 17, 2024 15:21
@Sbozzolo Sbozzolo added this pull request to the merge queue Sep 17, 2024
Merged via the queue into main with commit 418c59d Sep 17, 2024
14 of 16 checks passed
@Sbozzolo Sbozzolo deleted the gb/test_dependencies branch September 17, 2024 17:39
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.

3 participants