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

[libshortfin] Build debug/asan #149

Merged
merged 1 commit into from
Aug 26, 2024
Merged

[libshortfin] Build debug/asan #149

merged 1 commit into from
Aug 26, 2024

Conversation

marbre
Copy link
Collaborator

@marbre marbre commented Aug 23, 2024

This adds a GitHub action to build a debug/asan version of libshortfin. Shallow cloning of dependencies obtained via FetchContent is reverted as the dependencies cannot be fetched in the CI.

Depends on #148

@marbre marbre requested a review from stellaraccident August 23, 2024 19:33
Copy link
Contributor

@stellaraccident stellaraccident left a comment

Choose a reason for hiding this comment

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

Thanks. Left a number of comments but if it passes like this fine to land and iterate.

.github/workflows/ci_linux_x64_asan-libshortfin.yml Outdated Show resolved Hide resolved
.github/workflows/ci_linux_x64_asan-libshortfin.yml Outdated Show resolved Hide resolved
.github/workflows/ci_linux_x64_asan-libshortfin.yml Outdated Show resolved Hide resolved
pyenv global ${{ env.PYTHON_VER }}-debug
pyenv local ${{ env.PYTHON_VER }}-debug
pyenv shell ${{ env.PYTHON_VER }}-debug
pip install nanobind
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add this to requirements.txt and pin it (to 2.0). This can/has broken before and will poison the cache.

(should also add the requirements.txt and requirements-tests.txt hashes to the pyenv hash if installing these packages as part of the pyenv build)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the the dependency to the requirements-tests.txt and pinned the version to 2.0.0. As it also specified as a build-system dep with line
https://github.com/nod-ai/sharktank/blob/b1d09dbf1496949929c9aaf11f3cd9c551975958/libshortfin/pyproject.toml#L6
nanobind version 2.1.0 is installed when executing pip install -v -e. later. Should this be pinned to 2.0.0 as well?

Based on the work you've reviewed so far, I further improved caching. Instead of adding hashes for the requirements.txt files to the pyenv/python build step, I added an additional cache on top to cache the dependencies. Assuming that the dependencies are changing more rapidly, this will reduce the build time of the cache. The step to install and cache the dependencies takes about 60s whereas building pyenv/python takes ~ 5m 45s.

.github/workflows/ci_linux_x64_asan-libshortfin.yml Outdated Show resolved Hide resolved
libshortfin/CMakeLists.txt Outdated Show resolved Hide resolved
This adds a GitHub action to build a debug/asan version of libshortfin.
Shallow cloning of dependencies obtained via FetchContent is reverted as
the dependencies cannot be fetched in the CI.
@marbre
Copy link
Collaborator Author

marbre commented Aug 26, 2024

Thanks for the review and the suggestions @stellaraccident. With commit 7e7b77e, the ASan build fails but it succeeds without the commit, see for example run https://github.com/marbre/sharktank/actions/runs/10560961944/job/29255953907.

As it is not passing for main, please re-approve if you want me to land it.

@marbre marbre requested a review from stellaraccident August 26, 2024 14:08
@marbre marbre merged commit 40fac2a into nod-ai:main Aug 26, 2024
5 of 6 checks passed
@marbre marbre deleted the libshortfin-asan branch August 26, 2024 16:07
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.

2 participants