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

[DNMY] Improve CI performance. #303

Merged
merged 31 commits into from Jan 13, 2022
Merged

[DNMY] Improve CI performance. #303

merged 31 commits into from Jan 13, 2022

Conversation

euronion
Copy link
Contributor

Idea I came across which might make the CI faster.

Changes proposed in this Pull Request

Checklist

  • I tested my contribution locally and it seems to work fine.
  • Code and workflow changes are sufficiently documented.
  • Newly introduced dependencies are added to envs/environment.yaml and envs/environment.docs.yaml.
  • Changes in configuration options are added in all of config.default.yaml, config.tutorial.yaml, and test/config.test1.yaml.
  • Changes in configuration options are also documented in doc/configtables/*.csv and line references are adjusted in doc/configuration.rst and doc/tutorial.rst.
  • A note for the release notes doc/release_notes.rst is amended in the format of previous release notes.

@FabianHofmann
Copy link
Contributor

mmh, still the about same time right?

@euronion
Copy link
Contributor Author

Windows seemed faster, but maybe that's just a fluke. Still I'm not happy about the environment setup taking 15-20 minutes.

@euronion
Copy link
Contributor Author

Using environment.fixed.yaml failed. Do we indicate somewhere, for which applications (probably last stable release?) this file works?

@fneum
Copy link
Member

fneum commented Jan 12, 2022

It should be always tied to the latest release (that's when we update the environment.fixed.yaml.

@euronion
Copy link
Contributor Author

Some background info on why I am working on this atm:

CI is sometime very slow (>> 20min runtime) and sometimes slowish (10-20 min runtime). I'm trying to figure out what the reason for this is, as installation locally is not that slow. The CI spends large parts of the runtime (and most of it if it is very slow) in the installation of packages.

I came across an issue in the mamba development repository stating that mamba env update (which is used in the CI) used from within an env (and not the base env) falls back to using conda rather than mamba which would explain the long dependency resolving times.

I'll continue tinkering a bit with it.

The current version seems to run in ~5 min (Win, Linux) for dependency resolving+installation. That's significantly better performance. But we have already seen similar performance with the old ci.yaml config, so I would assume this is due to the performance of the runners rather than the code changes I've made so far.

@euronion
Copy link
Contributor Author

Constraining package versions understandably improves the performance noticeably (74c8b35), but I don't think we should prefer this approach.

@FabianHofmann
Copy link
Contributor

can it be that the dask version constraint slows things? It is still <=2021.3.1

@euronion euronion changed the title [DNMY] De- and reactivate test environment before mamba update. [DNMY] Improve CI performance. Jan 13, 2022
@euronion
Copy link
Contributor Author

can it be that the dask version constraint slows things? It is still <=2021.3.1

From what I understand constraints on the versions should make the resolve step faster, not slower. So probably not.

@euronion
Copy link
Contributor Author

Ok, latest version looks promising:

I am caching the resolved dependencies following the instructions provided by the setup-miniconda action for GitHub actions.

The dependencies are now only resolved (= cache is updated) if:

  • environment.yaml changes
  • Once a week when the CI runs following the weekly schedule
  • The cache is older than one day (last CI run on the previous day).
  • The value of the variable CACHE_NUMBER in ci.yaml changes.

Regarding performance:

  • If the environment has to be recreated (= cache updated), the performance is similar to a regular run. I've seen cache updates adding 20s to 2 minutes to the runtime.
  • If the environment does not need to be recreated (= cache can be used, conditions are listed above), the cache removes the need for resolving and installing the environment. Depending on the mood of the CI, this seems to shave off ~50% of the runtime.

@euronion euronion requested a review from fneum January 13, 2022 14:21
Copy link
Member

@fneum fneum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic! Thanks for going through all the effort! LGTM.

@fneum
Copy link
Member

fneum commented Jan 13, 2022

Would you like to squash it? If I do it, I think your contribution credit is lost.

@euronion
Copy link
Contributor Author

Will do! Thx!

Before that, I nearly forgot: I removed two things from environment.yaml:

  • mamba installation, because mamba isn't needed by pypsa-eur and mamba is not supposed to be installed in environments other than base
  • The gurobi channel, because the solver isn't installed, so that's not a real dependency

Hope that's fine?

@fneum
Copy link
Member

fneum commented Jan 13, 2022

Yes, sounds good! What's with the pending CIs? Does that come from the changed naming of CI runs?

@euronion
Copy link
Contributor Author

Yes, I believe that's due to renaming the runners.

@euronion
Copy link
Contributor Author

Ok, you do the honors @fneum . Because the name of the CI runners changed and the old ones are failing but compulsory, you (an Admin) has to disable them for the protected master branch. Also you'll probably have to change the names of the runners in the protected branch settings ;-)

@fneum fneum merged commit b660277 into master Jan 13, 2022
@fneum fneum deleted the misc/improve-ci-speed branch January 13, 2022 17:25
@fneum
Copy link
Member

fneum commented Jan 13, 2022

Done with updated branch protection rules :)

@euronion
Copy link
Contributor Author

👍

fneum added a commit that referenced this pull request Mar 6, 2023
add option to use electrolysis waste heat in district heating
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