Skip to content

Conversation

@Parskatt
Copy link
Collaborator

Resolves #72

Added build for multiple platforms. Also some fixes for platform specific issues.

Mac complained about "aligned-allocation" so compiling without it.
Windows broke because the MSVC flag was missing in the pybind cmakelists.

Currently no integration with the test suite, should add.
Also wheels seem to be sometimes broken, so we need to test these on some different systems before pushing pypi.

@Parskatt
Copy link
Collaborator Author

@pablospe Currently it will build on push to master, which I think is reasonable. I guess for other CI stuff we want to run on PRs, especially the test suite. Not sure how extensive we want to test, the current builds are for a wide range of setups, but that takes a very long time to run. Perhaps limiting the testing to just some ubuntu/python combo should be enough.

@pablospe
Copy link
Collaborator

In order to have a Github Action, you will need to set-up one first by clicking here:
https://github.com/PoseLib/PoseLib/actions/new

Then copy the files content of this PR to the new one, and close this PR to avoid confusion.

@pablospe pablospe mentioned this pull request Dec 5, 2023
@pablospe
Copy link
Collaborator

pablospe commented Dec 5, 2023

@vlarsson Is there any news about this PR?

@vlarsson
Copy link
Collaborator

vlarsson commented Dec 5, 2023

I will have a look at this next week after the 3DV camera ready deadline.

So this should upload to pypi when we make new releases? or I am not sure I am understanding the build-pypi.yaml.

@pablospe
Copy link
Collaborator

pablospe commented Dec 5, 2023

you will probably need to create a new github action, see my previous message:
#73 (comment)

@pablospe
Copy link
Collaborator

pablospe commented Mar 4, 2024

@Parskatt You might need to create a Github Action first, by clicking here:
https://github.com/PoseLib/PoseLib/actions/new

Then copy the files content of this PR to the new one. The github actions from build-pypi.yaml doesn't trigger otherwise, you can see it doesn't run in the "Checks" tab.

@Parskatt
Copy link
Collaborator Author

Parskatt commented Mar 4, 2024

Sorry, I did not mean to cause this review. Was just gonna fix stuff over at my fork. Perhaps we can disable the autoreview? I'll get back to you later.

@Parskatt Parskatt removed request for pablospe and vlarsson March 4, 2024 15:19
@pablospe pablospe marked this pull request as draft March 4, 2024 15:24
@Parskatt
Copy link
Collaborator Author

Parskatt commented Mar 15, 2024

@pablospe Ok, but the action won't run without the changes to rest in the PR, if I push the action first it will start running on pushes to master. Wouldn't that be kind of annoying? Maybe I'm misinterpreting you.

Also some general discussion:

  • I had to set BUILD_SHARED_LIBS manually off, since it seemed to mess with compatability.
  • Same for -march=native, had to remove it due to issues on mac @vlarsson . This is a bigger change. I ran the solver benchmarks, and I saw no diff in performance on the machine I tried. It could of course be worse in certain cases. I'm a super cmake noob, but it was also somewhat annoying that that benchmark binary ends up in a pile of build files, does files I have no idea where they would end up on the runners.
  • Also had to set -fno-aligned-allocation on mac to get stuff to compile.

@pablospe
Copy link
Collaborator

@pablospe Ok, but the action won't run without the changes to rest in the PR,

I was suggesting to create another PR where the actions are active, that's all, as now the build-pypi.yaml
is not running automatically, but the PR content should be the same, of course (only different PR). One question, how are you testing this? Your own fork/branch with manual triggering of the action?

If you see here:
image
only one github Actions is running and your .yaml is ignored. This is the reason I was suggesting to start a new PR where the action is working and you move the content (probably merging with the branch on this PR).

And one also wants to run it on master because it takes long time to run, and because you don't want to push artifacts to pypi from untested and non-approved branch.

if I push the action first it will start running on pushes to master. Wouldn't that be kind of annoying? Maybe I'm misinterpreting you.

Not sure what you mean. The action will run in the current branch on the PR, later you will need to filter and say the publishing of the artifact should run only in master, so when you publish a package you know it is from master, but this has to be done explicitly (it is not the default). Probably we are talking about something different?

Also some general discussion:

  • I had to set BUILD_SHARED_LIBS manually off, since it seemed to mess with compatability.

This is fine, just wondering how it works with the python bindings? Doesn't it needs a .so? (at least in linux)

  • Same for -march=native, had to remove it due to issues on mac @vlarsson . This is a bigger change. I ran the solver benchmarks, and I saw no diff in performance on the machine I tried.

You can also check the benchmark result when the action is running:
https://github.com/PoseLib/PoseLib/actions/runs/8214094373/job/22466214680?pr=73#step:7:72

@pablospe
Copy link
Collaborator

@Parskatt
Copy link
Collaborator Author

Parskatt commented Mar 19, 2024

@pablospe The issue is that we're building wheels, and the binaries don't end up in any convienent place as far as I can tell. In the basic CI that you already made, its easy to set the install_dir and simply run, but when building wheels I don't know where these targets end up.

Regarding testing the wheels, yes, they are already set up and running on my fork. I guess I can set up a new PR with your approach, so that the action runs there

@Parskatt
Copy link
Collaborator Author

@pablospe I followed your strategy above (see #95) , but this still only runs the already existing action (main.yml). I don't think its possible to run new CI in a PR.

@pablospe
Copy link
Collaborator

Now it is running, see the changes (you need to revert these before merging to PR, now it is for testing until you get it running):
7ac7e9f

So, it wasn't triggering because in your PR/branch because you added:

on:
  push:
    branches:
      - master

For testing you can comment it out, then when things are ready you can put it again so it only runs on master for the following PRs.

@pablospe
Copy link
Collaborator

should we close this PR? And move to the new one you created?

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.

pypi packaging and CI

3 participants