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.

Sign up for GitHub

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

Jump to bottom

python39Packages.opensfm: init at 0.5.2 #152957

Merged
merged 14 commits into from Mar 2, 2022
Merged

python39Packages.opensfm: init at 0.5.2 #152957

merged 14 commits into from Mar 2, 2022

Conversation

SomeoneSerge
Copy link
Contributor

@SomeoneSerge SomeoneSerge commented Dec 31, 2021 •
edited

Motivation for this change

Adds OpenSfM - an opensource structure from motion (reconstruction of a sparse 3d point cloud from images, similar to nixpkgs#colmap) framework maintained by Mapillary

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/672

@SomeoneSerge SomeoneSerge changed the title OpenSfM: init at 0.5.1 WIP: OpenSfM: init at 0.5.1 Jan 7, 2022
@SomeoneSerge SomeoneSerge marked this pull request as draft January 9, 2022 10:46
@SomeoneSerge
Copy link
Contributor Author

I've found this suffers from spurious segfaults when using HAHOG descriptors or when using multithreading or just randomly when computing depthmaps, so I'm converting this into a draft until I'm done with debugging

@SomeoneSerge SomeoneSerge changed the title WIP: OpenSfM: init at 0.5.1 OpenSfM: init at 0.5.2 Jan 17, 2022
@SomeoneSerge SomeoneSerge marked this pull request as ready for review January 17, 2022 19:36
@SomeoneSerge
Copy link
Contributor Author

No more disabled tests, and no segfaults!

I've bumped this to the (unreleased) 0.5.2 because the last release appeared to have some cxx standard issues that are only pronounced when using the new ceres (std::integer_sequence<>...), hotfixing which would've left a bunch of runtime errors.

I think the derivation is ready to be merged. Now that everything works I would reach out the upstream developers: hopefully they'd be willing to cooperate and remove the need for all the patches I've introduced. If that works out, I'll open a follow-up PR to remove the ad hoc hacks and ensure integration with nixpkgs-update

One more thing, I've been checking shebangs manually as follows:

nix build .#python3Packages.opensfm
./result/bin/opensfm_run_all ${opensfm-src}/data/berlin # modifies data/berlin in-place

(there's been a lot of issues with shebangs)

How should I automate this sort of a test?

@SomeoneSerge
python3Packages.opensfm: init at 0.5.2
f2464ea
@SomeoneSerge
python3Packages.opensfm: fetch patches from PRs
5be8b36
@SomeoneSerge
python3Packages.opensfm: limit platforms to linux
860a58d
@SomeoneSerge
opensfm: fpdf patch merged, bump revision
ef13b18
@SomeoneSerge
opensfm: move src and patches closer together
0190228
@SomeoneSerge
opensfm: use the nixpkgs-packaged pybind
d87c3a2 
This allows not to fetch the submodules.
Additionally, the original eigen cmake patch missed out ceres
(miraculously it worked for some opensfm revisions, and gave unresolved
symbols on others, at runtime).
@SomeoneSerge SomeoneSerge mentioned this pull request Feb 19, 2022
Closed
6 tasks
@SomeoneSerge
opensfm: back to nixpkgs-fmt
4803121
@SomeoneSerge
opensfm: eigen cmake target patch from PR
34befbf
@SomeoneSerge
opensfm: fix for darwin
83470f4 
- avoid having two opencvs in closure
- disable tests with lowerbounded residuals (smh darwin build converges
  to better minima - maybe different fp precision?)
@SomeoneSerge
opensfm: disable tests only for darwin
ac1e5d9
@SomeoneSerge
Copy link
Contributor Author

@SomeoneSerge
Copy link
Contributor Author

Actually, is it at all a good idea to have something as big and brittle as opencv4 in propagatedBuildInputs?

@SomeoneSerge @SuperSandro2000
opensfm: refactor platforms linux+darwin=unix
0413935 
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
@SuperSandro2000 SuperSandro2000 changed the title OpenSfM: init at 0.5.2 python39Packages.opensfm:+ init at 0.5.2 Mar 2, 2022
@SuperSandro2000 SuperSandro2000 changed the title python39Packages.opensfm:+ init at 0.5.2 python39Packages.opensfm: init at 0.5.2 Mar 2, 2022
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).

Result of nixpkgs-review pr 152957 run on x86_64-linux 1

2 packages built:
  • python310Packages.opensfm
  • python39Packages.opensfm

@SuperSandro2000 SuperSandro2000 merged commit c02944e into NixOS:master Mar 2, 2022
@SomeoneSerge
Copy link
Contributor Author

@SuperSandro2000 thank you for all of your comments and advice, this has been a pleasant journey

The derivation can still see some improvement, but I'll try to get the remaining patches upstream first!

facebook-github-bot pushed a commit to mapillary/OpenSfM that referenced this pull request Mar 10, 2022
@SomeoneSerge @facebook-github-bot
CMake: target_link_libraries() the native exported targets of Eigen a…
536b6e1 
…nd Ceres (#872)

Summary:
- [x] Upstream documentation suggests eigen be used through the `Eigen3::Eigen` target: https://eigen.tuxfamily.org/dox/TopicCMakeGuide.html
- [x] Eliminates the need for custom FindEigen.cmake and hard-coded paths
- [x] Makes it easier to package opensfm for distributions
- [x] Closes: #802
- [x] Checked `docker build -f Dockerfile.ceres2 .` and `nix build`
- [ ] `target_link_libraries` vs `target_include_directories` propagation rules: cf. bottom of the section https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#transitive-usage-requirements

I needed these changes in the process of packaging OpenSfM for nixpkgs, which significantly simplifies the building and deployment compared to plain docker: NixOS/nixpkgs#152957
You can see the build status, including the tests (currently failing to build for macos) at: https://github.com/SomeoneSerge/pkgs/actions

I'd hope to follow up with a few more changes that I found I needed, but I'd need your advice on how to go about them:
- Discovering system-distributed gtest?  #870
- Discovering system-distributed  pybind?  #871

Thank you!

Pull Request resolved: #872

Reviewed By: YanNoun

Differential Revision: D34521618

Pulled By: fabianschenk

fbshipit-source-id: 462b5c63f3c32c4d5fb9f105b7a52dd3b12635e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@jonringer jonringer Awaiting requested review from jonringer

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@SomeoneSerge @nixos-discourse @SuperSandro2000 @FRidh