Automatic installation of build/install requirements. #16
Conversation
hameerabbasi
force-pushed
the
pyproject-toml
branch
from
78025e5 to
091081d
Compare
|
If you want |
hameerabbasi
force-pushed
the
pyproject-toml
branch
from
091081d to
66db99d
Compare
I skipped the release job unless it was a |
hameerabbasi
force-pushed
the
pyproject-toml
branch
from
66db99d to
d53418a
Compare
| pull_request: | ||
| branches: | ||
| - main |
There was a problem hiding this comment.
I'd rather have a few tests at least on PRs, otherwise we might merge broken PRs.
There was a problem hiding this comment.
But keeping it would mess up ccache cache. We have limited resources from github runners and I would keep it only for actual valid runs, We should have separate run-test CI job in the future if we want to run any.
There was a problem hiding this comment.
But keeping it would mess up ccache cache.
Cache is not migrated from PRs to main, but cache from main can be used in PRs, as a security feature.
We have limited resources from github runners
We can use up to 20 concurrent runners, right now we're not even at 1. I think this mattered a lot more when setting up the builds.
We should have separate run-test CI job in the future if we want to run any.
This seems like a valid argument -- only test for a given version.
There was a problem hiding this comment.
I can work on adding a test job, but I need to clean things up a bit more.
There was a problem hiding this comment.
Some discussions should take place on how to approach it first, but I also think it isn't a priority right now, considering that we don't have virtually any tests. So I don't think it's worth spending time on right now.
There was a problem hiding this comment.
Existing job's purpose is to build LLVM and dialect wheel only. We don't have actual dialect feature yet so there's nothing to test yet.
| @@ -124,6 +127,7 @@ jobs: | |||
| release-wheel: | |||
| runs-on: 'ubuntu-latest' | |||
| needs: build-wheel | |||
| if: github.ref_name == 'main' | |||
There was a problem hiding this comment.
To remove before merging. I generally use this job to get wheels for PRs.
There was a problem hiding this comment.
You can get these at the bottom of a job as well, the only reason to have a release is to make the artifacts permanent:
There was a problem hiding this comment.
When I worked on the whole wheel build setup I found it more convenient/efficient to use release artifacts, rather than job artifacts.
|
Let's rerun it with |
hameerabbasi
force-pushed
the
pyproject-toml
branch
from
d53418a to
d515c55
Compare
|
It's weird -- I didn't get an email about this review. |
hameerabbasi
force-pushed
the
pyproject-toml
branch
from
16388d8 to
295b2bb
Compare
There was a problem hiding this comment.
@mtsokol Consider the commit 295b2bb optional -- to be accepted based on CI timings. It does clean a whole lot of gunk though, and locally
ccacheworks on the 2nd build.
ccacheworks on CI too, but may need an extra run to get the warm-cache timings.
Thanks for the cleanup! How about I will review it once there's another need to revise CI wheel setup? Then I'm keen to test it all locally also. But overall I really like the idea of making llvm-project a submodule.
On the ccache topic: GitHub offers 10GB of actions cache. For one PR it's ~1.5GB of cache saved each time it's triggered. So it's enough to push 6 commits to a single PR to completely exhaust the cache. Cache object are stored per branch and as of now there are no main related cache object (all got purged as it's LRU). So on monday the LLVM will be build from scratch already to build wheels: https://github.com/finch-tensor/Finch-mlir/actions/caches.
I think we still need to differentiate wheel build CI jobs (so what we have right now) that run on weekly basis, from future test CI that will only run dialect tests.
Considering that LLVM won't change often (because I think it's fine to upgrade it periodically when needed) I would even propose to store zip of LLVM build somewhere and make future test CI job download it, unzip, and only build the dialect. This way we will avoid messing up with cache and rebuild same LLVM version over and over again in the CI. I think working on a PR where CI takes either 30min or 2.5h (depending whether e.g. two people opened PRs at the same time and pushed something) is counterproductive. WDYT?
I can prepare such archive once we need it!
| @@ -158,16 +158,10 @@ def create_dir(name: str) -> Path: | |||
|
|
|||
|
|
|||
| setup( | |||
| name="finch-mlir", | |||
| version="0.0.2", | |||
There was a problem hiding this comment.
I'm happy to learn more arguments for it but I personally prefer git versioned version of the package, rather than rely solely on tags. As we do in finch-tensor-python and Finch.jl.
hameerabbasi
force-pushed
the
pyproject-toml
branch
4 times, most recently
from
7bae03a to
7979538
Compare
hameerabbasi
force-pushed
the
pyproject-toml
branch
from
7979538 to
42519cb
Compare
No description provided.