Skip to content

Add owl #341

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
Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Add owl #341

wants to merge 14 commits into from

Conversation

@moazzammoriani
Copy link
Contributor

@moazzammoriani moazzammoriani commented Apr 21, 2022

This is a partly concerning #331.

@moazzammoriani
Copy link
Contributor Author

moazzammoriani commented Apr 25, 2022 •
edited
Loading

After much hacking with the Makefile variable called PACKAGES and also adjusting the pin of the base package from v0.14.3 to v0.15.0--despite the fact that the version of base that owl depends on is unspecified--, I was able to at least get owl to show up in the list of packages to be installed before the build process. From this perhaps in can be inferred that base.v0.15.0 is required by owl rather than base.v0.14.3. However, owl is still failing to install because compilation of base is failing. A little research led me to discover that this is a known issue. Perhaps the solution then is to use an older version of owl @Sudha247 ?

@Sudha247
Copy link
Contributor

Sudha247 commented Apr 25, 2022

Thanks for the PR @moazzammoriani! Looking at the CI, you're spot on about the issue with base. Here's a patch to make it work with trunk opam pin add base.0.15.0 git+https://github.com/patricoferris/base.git#500+random. I think it's worth trying it, but please note that this is not an official version.

Perhaps the solution then is to use an older version of owl

I don't think that's going to work, unless the older version of owl doesn't depend on base. In any case, it's best to stick with the latest version of owl for accurate results.

@moazzammoriani
Copy link
Contributor Author

moazzammoriani commented Apr 26, 2022 •
edited
Loading

It appears that this patch doesn't quite suit the needs of this particular problem yet.

I did gain some insight though into why this patch for base didn't work. While testing the patched version of the package on my local environment, opam gave me the following message.

[base.0.15.0] synchronised from git+https://github.com/patricoferris/base.git#500+random
base is now pinned to git+https://github.com/patricoferris/base.git#500+random (version 0.15.0)

The following actions will be performed:
  ⊘ remove    owl      1.0.2*             [conflicts with base]
  ↘ downgrade sexplib0 v0.15.0 to v0.14.0 [required by base]
  ⊘ remove    stdio    v0.15.0            [conflicts with base]
  ↘ downgrade base     v0.15.0 to 0.15.0*
===== ↘ 2   ⊘ 2 =====

This implies that there are at least three points of failure:

  1. owl clearly doesn't work with it,
  2. sexplib0 needs to be pinned to v0.14.0, and I'm not sure that is possible since it's already pinned to a patched version (https://github.com/ocaml-bench/sandmark/blob/main/Makefile#L136),
  3. and stdio clearly doesn't play well with this patch either

moazzammoriani
moazzammoriani commented Apr 26, 2022
View reviewed changes
Makefile Outdated
opam pin add -n --yes --switch $* eventlog-tools https://github.com/ocaml-multicore/eventlog-tools.git#multicore
opam pin add -n --yes --switch $* coq-core https://github.com/ejgallego/coq/archive/refs/tags/multicore-2021-09-29.tar.gz
opam pin add -n --yes --switch $* coq-stdlib https://github.com/ejgallego/coq/archive/refs/tags/multicore-2021-09-29.tar.gz
opam pin add -n --yes --switch $* base.0.15.0 git+https://github.com/patricoferris/base.git#500+random
Copy link
Contributor Author

@moazzammoriani moazzammoriani Apr 26, 2022

Choose a reason for hiding this comment

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

Added the pin to the Makefile

@moazzammoriani moazzammoriani force-pushed the add_owl_and_ocaml-plplot branch 2 times, most recently from 7765618 to c447e0e Compare June 3, 2022 12:20
@moazzammoriani moazzammoriani changed the title Add owl and ocaml plplot Add owl Jun 8, 2022
@moazzammoriani moazzammoriani force-pushed the add_owl_and_ocaml-plplot branch from c447e0e to dc1ab2c Compare June 8, 2022 07:29
@moazzammoriani
Create opam files for new packages: owl and plplot
be08ce8
@moazzammoriani
Update opam files with links to tar file
74b6e01
@moazzammoriani
Setup simple test for owl
d7d51d4
@moazzammoriani
Update runparams
4bb4d4a
@moazzammoriani
Take step towards getting Owl dependencies without pinning to local repo
78d640d 
Remove unnecessary stanza from dune file.

Change url from local repo to whatever is on the opam file on the opam
repository.

Add list of dependencies that are actually being installed by Sandmark.
This list is useful because similar list in the README.md is inaccurate.
@moazzammoriani
Add owl dependencies
7ef03f9 
To dependencies/template/dev.opam:
- conf-openblas
- npy
- update stdio

To dependencies/packages:
- eigen
@moazzammoriani
Require owl dependencies to be installed
ef2fc01 
Add opam files for owl dependencies and dependencies of its
dependencies. Pin base to v0.15.0 in Makefile.
@moazzammoriani
Pin base to patched version
58b238c
@moazzammoriani
Arrive at a point where alt-ergo-lib is the only failing dep
787fd72
@moazzammoriani
Fix owl benchmark type error
d8bff85
@moazzammoriani
Fix benchmark bug and clear out redundant code and files
fcf0dbe
@moazzammoriani moazzammoriani force-pushed the add_owl_and_ocaml-plplot branch from dc1ab2c to fcf0dbe Compare June 8, 2022 09:39
@moazzammoriani moazzammoriani force-pushed the add_owl_and_ocaml-plplot branch from 65882cc to 59bf19b Compare June 8, 2022 14:02
@moazzammoriani moazzammoriani mentioned this pull request Jun 8, 2022
Open
@moazzammoriani moazzammoriani force-pushed the add_owl_and_ocaml-plplot branch from 9e73445 to 8370cea Compare June 9, 2022 07:13
shakthimaan
shakthimaan reviewed Jun 9, 2022
View reviewed changes
.github/workflows/main.yml
sudo apt-get -y install unzip aspcud libshp-dev libplplot-dev gfortran pkg-config git
git clone https://github.com/xianyi/OpenBLAS OpenBLAS
cd OpenBLAS/
make
Copy link
Contributor

@shakthimaan shakthimaan Jun 9, 2022

Choose a reason for hiding this comment

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

Where is make install step?

shakthimaan
shakthimaan reviewed Jun 9, 2022
View reviewed changes
.github/workflows/main.yml
cd OpenBLAS/
make
cd ..
sudo ldconfig /opt/OpenBLAS/lib/
Copy link
Contributor

@shakthimaan shakthimaan Jun 9, 2022

Choose a reason for hiding this comment

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

Why /opt, and why not /usr/local/lib? The other option is to install in /home/opam and update LD_LIBRARY_PATH.

Copy link
Contributor Author

@moazzammoriani moazzammoriani Jun 10, 2022

Choose a reason for hiding this comment

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

I'm actually not sure why /opt is used I just followed the Dockerfile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@shakthimaan shakthimaan shakthimaan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@moazzammoriani @Sudha247 @shakthimaan