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

Use cache for python modules to optimize CI build time #15050

Conversation

ArkadiuszMichalski
Copy link
Contributor

Partial implementation for #15028. This cause that XML validation in before_build and build_windows (Debug, Win32) job take less time (python modules will be restored from the cache). We save something like 20 seconds or more (depending on the real download time of these libraries). In the case of XML validation only (without compilation) the improvement is significant.

A short summary:

  • Only branch (push) saves the cache, so PRs can only download data (they cannot modify cache storage). Everything works without any problems on forks so everyone can test this solution.
  • The cache is automatically skipped/updated if there is a newer version of Python or the installed modules.
  • If any problems occur we can disable the whole thing through PYTHON_ALLOW_CACHE setting to false in CI_build.yml file. There is no need to revert commit, we can turn this off and wait to solve the problem.
  • If someone wants to skip the cache for some reason (mainly for testing purposes), just include the [force nopythoncache] flag in the commit title.

I did it separately because I don't know if I will be able to optimize the Scintilla/Lexilla compilation for all jobs (currently I have a working version only for MSYS2).

Write-Output "python=$key" >> $env:GITHUB_OUTPUT
}

- name: (cache) Lookup Python modules
Copy link
Contributor

Choose a reason for hiding this comment

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

You know the actions/setup-python Action has caching built in, right?

All you would need is a few lines:

  - name: Set up python
    uses: actions/setup-python@v5
    with:
      python-version: '3.12'
      cache: 'pip'

This way you're not stuck without whatever default Python version the Windows runner comes with, in case it falls out of date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it allows us to specify for which branches/events it should work for (I'm mainly concerned about saving)? I know that there are ready-made actions, but they usually have limited configuration options. For this reason I do not use the cache action itself, but separate save and restore (there is more precise control over how it should behave).

@donho donho self-assigned this May 1, 2024
@donho
Copy link
Member

donho commented May 1, 2024

From: #15028 (comment)

Okay, I found a way to cache only Scintilla/Lexilla, we need to slightly modify our makefile on the fly (in the same way as we change resource.h file).

@ArkadiuszMichalski
Though makefile is not used for the releases, there are still some people need it for compiling Notepad++. The aim of CI is to verify the state of code so IMO makefile should not be modified, even on the fly. However, I didn't see this part in the PR. So what has been cached exactly? Only the python module?

@ArkadiuszMichalski
Copy link
Contributor Author

@donho Yes, here we only cache python libraries that are used for XML validation (requests rfc3987 pywin32 lxml). I did it separately, because if Scintilla/Lexilla optimization will be useless (or not accepted), at least these libraries can be cached (we use them in two places, so it seems unnecessary to install them over and over again).

@donho
Copy link
Member

donho commented May 1, 2024

@chcg
Do you think it's maintainable if the PR goes into the master?

@donho
Copy link
Member

donho commented May 3, 2024

@ArkadiuszMichalski
After comparing the result of the PR with the master build time, I noticed some build time (Win Debug x64) are even higher than the master's ones.

So I tried to launch 1 time for having the cache, then the 2nd time (almost immediately) for using the cache.
Here the 2nd build result:

CI_build / before_build (pull_request) Successful in 15s
CI_build / build_windows (Release, x64) (pull_request) Successful in 4m
CI_build / build_windows (Release, Win32) (pull_request) Successful in 4m
CI_build / build_windows (Release, ARM64) (pull_request) Successful in 4m
CI_build / build_windows (Debug, x64) (pull_request) Successful in 7m
CI_build / build_windows (Debug, Win32) (pull_request) Successful in 4m
CI_build / build_windows (Debug, ARM64) (pull_request) Successful in 3m
CI_build / build_windows_cmake (Release, x64, amd64) (pull_request) Successful in 4m
CI_build / build_windows_msys2 (Release, x86_64) (pull_request) Successful in 5m
CI_build / build_windows_msys2 (Release, i686) (pull_request) Successful in 5m
CI_build / build_windows_msys2 (Debug, x86_64) (pull_request) Successful in 5m
CI_build / build_windows_msys2 (Debug, i686) (pull_request) Successful in 4m 

vs master last commit build:

CI_build / before_build (push) Successful in 24s
CI_build / build_windows (Release, x64) (push) Successful in 3m
CI_build / build_windows (Release, Win32) (push) Successful in 3m
CI_build / build_windows (Release, ARM64) (push) Successful in 3m
CI_build / build_windows (Debug, x64) (push) Successful in 5m
CI_build / build_windows (Debug, Win32) (push) Successful in 3m
CI_build / build_windows (Debug, ARM64) (push) Successful in 3m
CI_build / build_windows_cmake (Release, x64, amd64) (push) Successful in 4m
CI_build / build_windows_msys2 (Release, x86_64) (push) Successful in 5m
CI_build / build_windows_msys2 (Release, i686) (push) Successful in 4m
CI_build / build_windows_msys2 (Debug, x86_64) (push) Successful in 4m
CI_build / build_windows_msys2 (Debug, i686) (push) Successful in 4m

As you can see, the result is not promising. Or the cache can be enabled in master? If it's the case, how can we test to see the gain of the PR?

@ArkadiuszMichalski
Copy link
Contributor Author

ArkadiuszMichalski commented May 3, 2024

@donho
Such a comparison of times will never be accurate because these times are always different, probably depending on the current load of the machine. Check the last few master commits in the main repo and you will notice that sometimes we get such slow jobs. You can share a link to this slow job and we'll see what took so long.

We use cache only for before_build if we only change the XML file (without compilation necessary) or Win32 Debug (when we change the XML file that requires compilation, basically we always use cache for this job because of testing). It's best to analyze only these two builds. Maybe this way:

  1. On fork update master
  2. Add my change from this PR to master (after doing the push, the cache should be created because it not exist)
  3. Now change some files that require XML test (with/without compilation necessary) on master or make new branch (cache should be used).
  4. You can also send PR from fork to fork to see if the cache will be invoked correctly for PR (e.g. if it did not exist, PR should not create it, because I assumed that PR can only restore the cache).

On Action page you can manually delete existing cache and repeat test or add on commit tittle [force nopythoncache] so cache will be skipped for this commit if it exists.

But it's still possible that sometimes Win32 Debug with cache will be slower than without cache (but this is caused by other factors, e.g. in the first case it takes longer to start the container, perform checkout, or other tasks whose times are always vary).

@ArkadiuszMichalski
Copy link
Contributor Author

ArkadiuszMichalski commented May 4, 2024

@donho In this PR in before_build job we have separate step for installing python modules (if cache does not exist). You can commit a few times to some XML file (when the cache does not exist), which will only perform validation to see how long it takes (we can determine the average download and installation of Python modules). This time may vary, but I noticed somewhere around 15-30 seconds.

image

With the cache we reduce it to 3-5s, because that's how long it takes to restore the cache.

image

So for example if the before_build or Win32Debug job without cache may takes:

1 before_build 30s
2 before_build 35s
3 before_build 40s

4 Win32Debug 3min 50s
5 Win32Debug 4min
6 Win32Debug 6min (for some random factor it takes so long)

with cache the same jobs will be always faster:

1 before_build (30s - (time to download and install python modules) + (time to restore cache))
2 before_build (35s - (time to download and install python modules) + (time to restore cache))
3 before_build (40 - (time to download and install python modules) + (time to restore cache))

4 Win32Debug (3min 50s - (time to download and install python modules) + (time to restore cache))
5 Win32Debug (4min - (time to download and install python modules) + (time to restore cache))
6 Win32Debug (6min - (time to download and install python modules) + (time to restore cache))

This may not be a spectacular result, but we save some energy and eliminate unnecessary network bandwidth.

@donho
Copy link
Member

donho commented May 10, 2024

@ArkadiuszMichalski
It'll be merged into master. However, if the performance is not improved comparing the build time (after merging) of previous ones before this PR being merged, the commit will be revert, for the sake of KISS.

@donho donho added the accepted label May 10, 2024
@donho donho closed this in be94533 May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants