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 artifacts to cache the build #51

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Use artifacts to cache the build #51

wants to merge 12 commits into from

Conversation

sebbASF
Copy link
Contributor

@sebbASF sebbASF commented Jun 11, 2024

The PR looks for a GFM build artifact in the repo housing the action.
If one is found, it is downloaded and the build is skipped.

It can take 15+ seconds to build GFM, which is large proportion of the run-time, so it seems worth doing.

[A previous version of this PR also checked the calling repo for the artifact, and would create an artifact if none was found there either. However this would potentially create artifacts in lots of repos, as well as being an unexpected side effect of the build action]

@sebbASF sebbASF marked this pull request as draft June 11, 2024 23:10
@sebbASF
Copy link
Contributor Author

sebbASF commented Jun 11, 2024

Also need a job to create the artifact in the action repo when necessary.
This is provided by #56

@sebbASF sebbASF requested a review from gstein June 15, 2024 13:53
@sebbASF sebbASF marked this pull request as ready for review June 15, 2024 13:53
Ensure LIBCMARKDIR uses WORKDIR
@gstein
Copy link
Member

gstein commented Jun 15, 2024

Should we create a distinct action for this? eg. infrastructure-actions/build-gfm ? ... It seems there is a fair bit of logic around this, which could be pulled out into a separate action for clarity of maintenance. ... I don't have a particular opinion either way. The current action.yml isn't so large as to be confusing.

All that said, I do think that caching the artifact is the right approach. No need to keep rebuilding it for $N sites and runs.

@gstein
Copy link
Member

gstein commented Jun 15, 2024

Oh. I just saw your reference to #56 ... but my query is: why duplicate build logic between that and this one? And #56 has a very different looking code for a build (it has a lot of cron/expiration checks) ... could we roll up all logic around building/stashing the artifact into a new Action. ?

@sebbASF
Copy link
Contributor Author

sebbASF commented Jun 15, 2024

AFAICT, actions don't have a way to exit cleanly early.
One would need to set a flag and check it in every subsequent step of the action.
[Or use a hack and exit with an error, but catch it at the end and force a successful exit. Ugh!]

Also an action can only be called as a separate step; it cannot be invoked as part of a step that has a run clause.

So I cannot see any way to extract an action that would simplify the code.

However, I think the build sections could be simplified slightly by moving the pushd $WORKDIR/popd statements into build-cmark.sh

Note also that the build-artifacts job is only intended for use on the actions repo, so I'm not sure it helps to extract some of the logic into a separate action.

@gstein
Copy link
Member

gstein commented Jun 15, 2024

build-cmark.sh is fair game, as it is only used within this repository/action. Use your best judgement. Thanks!

@gstein
Copy link
Member

gstein commented Jun 15, 2024

Offhand, let the Action define the LIBCMARKDIR, and hand that to build-cmark.sh. Is that your thinking?

@sebbASF
Copy link
Contributor Author

sebbASF commented Jun 15, 2024

Offhand, let the Action define the LIBCMARKDIR, and hand that to build-cmark.sh. Is that your thinking?

That would require other changes to build-cmark.sh, as it would potentially have to move the lib directory to the input location.
So I did not do that.

However it now occurs to me that it would simplify the Docker build if it could specify LIBCMARKDIR without having to know the directory structure of the tar file. Only the build file should know that.

I might change it again...

@gstein
Copy link
Member

gstein commented Jun 15, 2024

At a minimum, build-cmark.sh could simply mv cmark.so $LIBDIR_GIVEN_TO_US

@sebbASF
Copy link
Contributor Author

sebbASF commented Jun 15, 2024

At a minimum, build-cmark.sh could simply mv cmark.so $LIBDIR_GIVEN_TO_US

I think it may need the whole of the lib directory, but that is no harder to move.

Comment on lines +81 to +91

# Is there a saved artifact for the GFM build?
echo "Check for GFM build artifact in action repo: $GITHUB_ACTION_REPO"
gh run download --dir ${LIBCMARKDIR} --name ${GFM_ARTIFACT_KEY} --repo $GITHUB_ACTION_REPO || true
if [[ -f $LIBCMARKDIR/libcmark-gfm.so ]]
then
echo "Downloaded to ${LIBCMARKDIR} from $GITHUB_ACTION_REPO, nothing more to do!"
exit 0 # nothing more to do in this step
fi

# GFM binary not found, need to build it
Copy link
Member

@assignUser assignUser Jun 18, 2024

Choose a reason for hiding this comment

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

In contrast to caching compile caches (see #32 ) this is actually a good/intended use case for https://github.com/actions/ cache because you have the fixed key of the gfm version and don't need any incremental updates.

The cache action also handles scoping of the cache so that a run of the action on the default branch won't pull in an artifact from a (user supplied, thus untrusted) fork PR CI run. You can then simply do this in an earlier step:

- name: Cache GFM
  id: gfm-cache
  uses: actions/cache@v4
  with:
    path: ${{ env.LIBCMARKDIR }}
    key: gfm-lib-${{ env.GFM_VERSION }}

and use the output steps.gfm-cache.outputs.cache-hit in an ${{ }} expression to check if the lib was restored, if it was not the action will automatically cache path: at the end of the action. And the cache doesn't expire, it will only be LRU evicted if the 10GB/repo limit is reached so #56 would also no longer be necessary.

However this would potentially create artifacts in lots of repos, as well as being an unexpected side effect of the build action

Actions making use of the gh provided cache functionality is pretty much expected imo and shouldn't be an issue especially with such a tiny cache but an option to disable caching could be provided.

Copy link
Member

Choose a reason for hiding this comment

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

(some people recommend pinning even actions/* actions to specific shas in jobs with elevated permissions but that is currently not required by the gha policy)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the notification spam. Is pelican not compatible with libcmark-gfm 0.29.0 which is available via apt in both 20.04 and 22.04?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the notification spam. Is pelican not compatible with libcmark-gfm 0.29.0 which is available via apt in both 20.04 and 22.04?

When I tested it, I found differences in the output.
Also, changing from BuildBot to GH CI is a big change, and the fewer other changes that are made, the easier it is to debug problems. Updates to GFM and Pelican versions can be made later.

@sebbASF
Copy link
Contributor Author

sebbASF commented Jun 18, 2024

AFAICT the apt version of GFM is designed as a stand-alone executable.
There is an associated library, but it is not in the layout expected by the ASF gfm plugin.
Either the layout would have to be emulated by the action, or the plugin needs work to use the executable (which might be slower?)

I think this is job for later once the action is known to be working OK.

It also appears to be a bit slower than fetching an artifact.

(Later) I tried installing it and setting up the expected links.
However the gfm plugin fails with:
libcmark-gfmextensions.so: undefined symbol: core_extensions_ensure_registered
I doubt it is worth trying to fix that.

@sebbASF
Copy link
Contributor Author

sebbASF commented Jun 18, 2024

One more consideration:

Using actions/cache doesn't help with the first build in a new repo (not sure if it helps with the first build when a new branch is cloned), whereas the cached artifacts are always available to new branches and new repos.

@assignUser
Copy link
Member

Also, changing from BuildBot to GH CI is a big change, and the fewer other changes that are made, the easier it is to debug problems. Updates to GFM and Pelican versions can be made later.

👍 makes sense, was just wondering :)

whereas the cached artifacts are always available to new branches and new repos.

True but they also add a considerable amount of code/process to maintain and with the build only taking 15 seconds I'd lean towards 'no code is the best code' ^^ If the build was more substantial, this would be a great setup, but for 15 seconds it seems a bit overkill. An alternative would be to use a docker image in the step to run pelican, or even a complete docker action but I don't really have any experience with Pelican so 🤷 ghcr.io caching + a minimized image makes that pretty fast too.

not sure if it helps with the first build when a new branch is cloned

If there is a cache on the base branch then yes.

@sebbASF
Copy link
Contributor Author

sebbASF commented Jun 26, 2024

I tried using a Docker image and that was slower.

@assignUser
Copy link
Member

I tried using a Docker image and that was slower.

To clarify before I go down a rabbit hole to test this: you used the docker file and build the image in the workflow or pre-built a docker image and hosted it on ghcr.io and used that in the workflow?

@sebbASF
Copy link
Contributor Author

sebbASF commented Jul 11, 2024

I think I used ghcr.io. This took quite a while to load.

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