Skip to content

feat: integrate dev branch and update software environments (#25) #29

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

Closed
wants to merge 1 commit into from

Conversation

btraven00
Copy link
Contributor

  • run from post-0.2.0 tag, main branch
  • docs: use public repo URIs
  • chore: add convenience target to build environments
  • add top-level Makefile to prepare env
  • feat: parametrize num of cores on the makefile
  • chore: ignore common temporary outputs and image build artifacts
  • update .eb files to easybuild 5.0
  • remove remote storage
  • do not run artifacts if not in main repo
  • inject checksums to rmarkdown easyconfig
  • update sklearn singularity definition
  • feat: add microbenchmark for numpy operations
  • chore: bump clustering-benchmarks to 1.1.6
  • feat: templatize the definitions
  • feat: mv output folders to timestamped names
  • feat: add --yes flag
  • docs: update README

…hmark#25)

* run from post-0.2.0 tag, main branch
* docs: use public repo URIs
* chore: add convenience target to build environments
* add top-level Makefile to prepare env
* feat: parametrize num of cores on the makefile
* chore: ignore common temporary outputs and image build artifacts
* update .eb files to easybuild 5.0
* remove remote storage
* do not run artifacts if not in main repo
* inject checksums to rmarkdown easyconfig
* update sklearn singularity definition
* feat: add microbenchmark for numpy operations
* chore: bump clustering-benchmarks to 1.1.6
* feat: templatize the definitions
* feat: mv output folders to timestamped names
* feat: add --yes flag
* docs: update README
@imallona imallona self-requested a review May 22, 2025 05:51
Copy link
Member

@imallona imallona left a comment

Choose a reason for hiding this comment

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

this is not a request for changes, just a proactive way to block the merge until I check sometime next week

note to self: red tests; singularity recipes to be made reproducible and planned with a grid; what overrides, ; update/upgrade actions; check how microbench, overrides, smoketests etc are supposed to be used; agree on a registry with shared write rights etc

@imallona
Copy link
Member

Can I cherrypick the rmarkdown.eb @btraven00 to main while also blocking the merge? To me the PR is promising but not timesaving (too many changes including the removal of comments or docs, and other changes that invalidate old runs, such as the fcps easyconfig graph version upgrade)

@btraven00
Copy link
Contributor Author

As mentioned elsewhere, let me know which parts you want split and I'll do a new PR.

@btraven00
Copy link
Contributor Author

btraven00 commented May 28, 2025

and other changes that invalidate old runs, such as the fcps easyconfig graph version upgrade

Do you mean you want me to revert this minor version bump? https://github.com/omnibenchmark/clustering_example/pull/29/files#diff-a6cca9b20e49ee9b099e3d6c9b2a7fe6712bb85b90931cf71e2e3160b08fc5eaR188

TBH I didn't fully grasp the argument (what old runs are we discussing here?). Feel free to request the revert, but be aware that it's not guaranteed to build without it (hint: bioconductor versions) 😄

Copy link
Member

Choose a reason for hiding this comment

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

we switch to clustering_[conda|apptainer|envmodules].yaml only, and they're short, all others are out
we test on PR and nightly (short and long), perhaps with timeout
we don't write artifacts
we don't test reduced-imallona branch because it's not relevant anymore
we switch to the new run_omnibenchmark action

Copy link
Member

Choose a reason for hiding this comment

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

same as above, and replace by the run_ob action

Copy link
Member

Choose a reason for hiding this comment

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

same as above, and replace by the run_ob action

Copy link
Member

Choose a reason for hiding this comment

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

let's automate writing to the registry and tag the images with the last (?) commit or some other identifier to be able to track versions

decide on an oras registry and share credentials/omnibenchmarkteam-wise, check quotas

get mark to do the bitwarden/MLS omnibenchmark credentials storage/thing

Copy link
Member

Choose a reason for hiding this comment

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

delete it, see above

Copy link
Member

Choose a reason for hiding this comment

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

delete

Copy link
Member

Choose a reason for hiding this comment

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

delete

Copy link
Member

Choose a reason for hiding this comment

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

delete

Copy link
Member

Choose a reason for hiding this comment

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

delete

Copy link
Member

Choose a reason for hiding this comment

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

perhaps combine the R envs into one, and avoid compiling another R

Copy link
Member

Choose a reason for hiding this comment

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

delete

Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Member

Choose a reason for hiding this comment

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

delete, same

Copy link
Member

Choose a reason for hiding this comment

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

this goes to the omnibenchmark_paper repo and not to this one, here the makefile should be easier

2. Clone the benchmark definition / this repository with `git clone [email protected]:omnibenchmark/clustering_example.git`
3. Move to the cloned repository `cd clustering_example`
4. Run locally, somewhat in parallel `ob run benchmark -b CLUSTERING.YAML --local --threads 6`. Choose `Clustering.yml` specification based on whether running it with conda, easybuild, apptainer, etc. [More details about the available backends](https://github.com/omnibenchmark/clustering_example/blob/main/envs/README.md).
1. Install omnibenchmark: `pip install omnibenchmark>=0.2.0`
Copy link
Member

Choose a reason for hiding this comment

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

conda is needed, lmod, envmodules etc - not only pip


by Marek Gagolewski, modified by Izaskun Mallona
* All needed recipes can be found under `envs`: conda, apptainer, easybuild (lmod modules)
Copy link
Member

Choose a reason for hiding this comment

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

for the example this is way too complicated, let's move this to the paper repo

Copy link
Member

Choose a reason for hiding this comment

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

same, delete, and make sure singularity is reproducible and called apptainer

Copy link
Member

Choose a reason for hiding this comment

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

make sure the easyconfigs are merged/reproducible, discuss the conda dependency, perhaps mamba

Copy link
Member

Choose a reason for hiding this comment

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

too many things to unpack - let's test and pick one of the two alternatives
but if versions are changing (package versions) then conda's and apptainer's should match

Copy link
Member

Choose a reason for hiding this comment

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

same. btw we might need to document the bootstrap thing

Copy link
Member

Choose a reason for hiding this comment

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

same, rm

Copy link
Member

Choose a reason for hiding this comment

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

same, not repro

Copy link
Member

Choose a reason for hiding this comment

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

discussed why this sounds risky to me

Copy link
Member

Choose a reason for hiding this comment

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

add versions, if not merged to fcps

@btraven00
Copy link
Contributor Author

after a review today, we'll be closing this PR in favor of #30
part of this PR will be made against https://github.com/omnibenchmark/omnibenchmark_paper, we want to leave a clean and minimal example as reference.

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.

2 participants