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.

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

adding nixpkgs test workflow #46

Closed
wants to merge 1 commit into from

Conversation

AtilaSaraiva
Copy link
Contributor

Hello again!

As mentioned in my comment at issue #45, it is possible to add a new workflow that actually tries to build deepwave with nix. This should help avoiding problems the nixpkgs derivation having tests fail with each PR.

@ar4
Copy link
Owner

ar4 commented Jul 20, 2022

Nice! Thank you for doing this.

I have never used Nix before. On this line you specify a version of Deepwave. In the example on the Nix wiki they don't specify a version, but do specify a source. Is there an advantage to specifying a version?

The Actions documentation seems to use the syntax on: [push, pull_request] to specify multiple triggers.

Version 3 of the GitHub checkout Action is available. Is there a reason to prefer v2.4.0?

Interestingly, the testing seems to take longer with this Nix approach. The regular Action only takes about 26 minutes to run all of the tests, but the tests take 37 minutes in this Nix Action. It seems to be installing a recent version of GCC (11.3), so poor optimisation is probably not the cause.

@AtilaSaraiva
Copy link
Contributor Author

AtilaSaraiva commented Jul 20, 2022

No problem!

Maybe the wiki is outdated, but now there is a need to specify the version, look what happens when I don't:

$ nix-build
error: attribute 'version' missing

       at /nix/store/8lvs5f6qw3p7rj8rjg293y353qac9qyq-n147730f80lz7smljqxjzyfch62imgix-source/pkgs/development/interpreters/python/mk-python-derivation.nix:30:28:

           29|
           30| { name ? "${attrs.pname}-${attrs.version}"
             |                            ^
           31|
(use '--show-trace' to show detailed location information)

you can always put version="master"; though.

As for the syntax of the github actions and the checkout version, I simply copied the example config from the install nix workflow haha, I will abide by your suggestions. I'm quite new to writing workflows.

As for the speed of the job, I have no clue why it is slower. As a wild guess, I figure that this could be happening because python has optimizations turned off by default on nix, to ensure reproducibility. On my machine it is quite fast specially because it can use multiple cores, and I'm confident that the derivation (the default.nix file) is optimized to close to maximum performance without taking the python optimizations into account. I will check how we could enable those to test if there is a significant performance increase. but do keep in mind that the optimized python will have to be compiled. Another thing that could be happening is some sort of I/O bottleneck since idk how optimized for nix this ubuntu image is, but there isn't a nixos base image for github actions for us to test.

@ar4
Copy link
Owner

ar4 commented Jul 20, 2022 via email

@AtilaSaraiva
Copy link
Contributor Author

AtilaSaraiva commented Jul 20, 2022

I'm pushing a test commit for us to investigate the possibility of using a optimized python. However, this could even take longer because of the python compilation. One alternative is to use cachix-action to cache the python build between workflow runs, I can even pin a nixpkgs revision so that it doesn't change the python derivation too often.

For us to use the cachix-action workflow we can follow this tutorial: nix.dev/tutorials/continuous-integration-github-actions. For it to work a cachix account can be created (it would be better for you to do that part), and a auth key should be added to the repository secrets, then I'd only add the cachix config.

This usage of the cachix even has the potential of reducing the time taken to download dependencies since they will be shared between workflow runs.

@ar4
Copy link
Owner

ar4 commented Jul 20, 2022 via email

@AtilaSaraiva
Copy link
Contributor Author

Yeah, I agree, it is nice to have the reproducibility stuff to ensure that if the tests pass here they will pass for everyone with nix.

@ar4
Copy link
Owner

ar4 commented Jul 20, 2022

It looks like it took 43 minutes to run the Nix Action with the optimised Python, so not any faster, but interestingly I see that in your run of the regular Action the tests also took 44 minutes, so maybe something else has changed recently that is causing it to run more slowly (since the tests ran consistently in around 26 minutes in the few commits since I last changed the code), such as some other package being updated or GitHub switching to slower instances.

@AtilaSaraiva
Copy link
Contributor Author

Interesting, so mistery solved haha. I will drop the last commit then for us to rollback to the reproducible build of python.

@ar4
Copy link
Owner

ar4 commented Jul 21, 2022

I slightly increased the tolerance for that test (from 1e-8 to 5e-8) and now it passes, so have merged it. Thank you again.

@AtilaSaraiva AtilaSaraiva deleted the nix_testbed branch July 21, 2022 18:52
@AtilaSaraiva
Copy link
Contributor Author

No problem whatsoever! Thank you for being so nice about this whole thing, maintainer and dev relationships are not always this cool, haha.

@ar4
Copy link
Owner

ar4 commented Jul 27, 2023

Hello @AtilaSaraiva

In response to some users having difficulty with the way Deepwave was previously distributed, which involved code being compiled on their system, I have now switched to using GitHub Actions to precompile the code into shared/dynamic libraries that I now distribute. I hope that this will avoid problems for most users. I am not sure how this affects your Nix package, though. If you have any questions about how it works, or suggestions on how to improve it (including how it could be made easier for you to maintain the Nix package) please let me know.

@AtilaSaraiva
Copy link
Contributor Author

AtilaSaraiva commented Jul 27, 2023 via email

@ar4
Copy link
Owner

ar4 commented Jul 27, 2023 via email

@AtilaSaraiva
Copy link
Contributor Author

AtilaSaraiva commented Jul 27, 2023 via email

@ar4
Copy link
Owner

ar4 commented Jul 28, 2023 via email

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