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

Feat/build profiles #381

Closed
wants to merge 5 commits into from
Closed

Feat/build profiles #381

wants to merge 5 commits into from

Conversation

0xJepsen
Copy link
Contributor

Closes #370

Left a bunch of notes in the issue that describe my results. tldr is that this should cut off about 6 minutes of CI

@0xJepsen 0xJepsen requested a review from Autoparallel January 16, 2025 21:14
@mattes
Copy link
Contributor

mattes commented Jan 17, 2025

Fixed the little merge conflict. But you will have to rebase with main to fix the notary/deploy issue.

@mattes
Copy link
Contributor

mattes commented Jan 17, 2025

Building "notary --debug" in the notary workflow means we will deploy a debug version to the staging instance. I'm torn if that's cool or not. Thoughts? Presumably at some point in the future we'd like to attach a prebuilt version of the "notary --release" to the Github Release?

I noticed we are only using macos-latest for client. Should we bring back support for ubuntu-latest as well?

@0xJepsen
Copy link
Contributor Author

Presumably at some point in the future we'd like to attach a prebuilt version of the "notary --release" to the Github Release?

hmm this is a good thought. I think maybe debug version is good for now. My thoughts here are that the notary isn't performance bound so the release flag doesn't do a whole lot for it here but it does increase the build time by about 3minutes. I do like the idea of at somepoint in the future maybe after #380 and the back-end test-net project we can be in a better place to do that? What do you think?

@0xJepsen 0xJepsen force-pushed the feat/build_profiles branch from e81e513 to 8e402e4 Compare January 17, 2025 16:44
@0xJepsen
Copy link
Contributor Author

Gave this a rebase not sure what needs to happened here.

@@ -49,7 +49,8 @@ jobs:
path: proofs/web_proof_circuits
key: ${{ needs.setup.outputs.cache-key }}

- run: cargo +nightly-2024-10-28 build -p notary --release
- run: cargo +nightly-2024-10-28 build -p notary
- uses: Swatinem/rust-cache@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove - uses: Swatinem/rust-cache@v2 here, this cache action needs to run before the actual build to pull in the cached files. This is currently already happening in line 33.

@mattes
Copy link
Contributor

mattes commented Jan 17, 2025

This artifact action is expecting a --release built. But this PR switches it to a debug built. That's why the notary deploy action fails, it can't find the correct file.

Write artifact here: https://github.com/pluto/web-prover/pull/381/files#diff-cf936e8fd1e64fac1eca112295f2ebdd20f5e01f0ce2b7a95f59c2be07a24a08L58

Read artifact here: https://github.com/pluto/web-prover/pull/381/files#diff-cf936e8fd1e64fac1eca112295f2ebdd20f5e01f0ce2b7a95f59c2be07a24a08R78

@mattes
Copy link
Contributor

mattes commented Jan 17, 2025

I just remembered something, TLS Notary in debug build does not work. Proof times are too slow. So it actually probably the case we need to run the release built in staging as well.

cc @devloper @Autoparallel

@0xJepsen 0xJepsen closed this Jan 22, 2025
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.

chore: Notary shouldn't need to always run in release profile since proving is client side
2 participants