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

devito: init at unstable-2022-04-22 #165484

Merged
merged 4 commits into from
Apr 28, 2022
Merged

Conversation

AtilaSaraiva
Copy link
Contributor

@AtilaSaraiva AtilaSaraiva commented Mar 23, 2022

Description of changes

I implemented this package because I going to need it for my job. Devito is a domain specific language API that allows for code generation of partial differential equations solutions, and is highly optimized for both CPU and GPU.

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.

@AtilaSaraiva AtilaSaraiva changed the title Devito devito: init at 4.6.2-bbad3b1 Mar 23, 2022
@ratsclub ratsclub self-requested a review March 23, 2022 19:01
Copy link
Member

@ratsclub ratsclub left a comment

Choose a reason for hiding this comment

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

LGTM

@AtilaSaraiva
Copy link
Contributor Author

Thx for the review!

@liff
Copy link
Contributor

liff commented Mar 27, 2022

The packages should have meta.maintainers set.

@AtilaSaraiva
Copy link
Contributor Author

The packages should have meta.maintainers set.

Oh I sincerily forgot, I gonna fix that right away. Btw, do you know a way for me to ammend previous commits without messing up the tree?

@liff
Copy link
Contributor

liff commented Mar 27, 2022

You could do an interactive rebase:

$ git rebase -i master

Then mark all of the commits edit. When you save and quit the editor, you’ll get dropped into a shell and git should give you instructions on how to proceed.

@AtilaSaraiva
Copy link
Contributor Author

You could do an interactive rebase:

$ git rebase -i master

Then mark all of the commits edit. When you save and quit the editor, you’ll get dropped into a shell and git should give you instructions on how to proceed.

Thx, gonna try it!

pkgs/development/python-modules/pyrevolve/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/devito/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/devito/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/devito/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/devito/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/codepy/default.nix Outdated Show resolved Hide resolved
@AtilaSaraiva AtilaSaraiva changed the title devito: init at 4.6.2-bbad3b1 devito: init at 4.6.2 Apr 4, 2022
@AtilaSaraiva
Copy link
Contributor Author

Ok, I think I covered almost all of the requests. What is left is running the tests.

@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Apr 12, 2022
@AtilaSaraiva AtilaSaraiva force-pushed the devito branch 3 times, most recently from 9c40e47 to de15125 Compare April 12, 2022 16:26
@AtilaSaraiva
Copy link
Contributor Author

@SuperSandro2000 have you taken a look into the test errors for devito too, in the pastebin I linked? Should I set doCheck=false too for them?

@SuperSandro2000
Copy link
Member

Oh, forgot that.

I don't know mpi or how to fix the error.

The value of the MCA parameter "plm_rsh_agent" was set to a path
that could not be found:

  plm_rsh_agent: ssh : rsh

Please either unset the parameter, or check that the path is correct

I would suggest to add the 4 tests to disabledTests which together with pytestCheckHook ignores the tests.

@AtilaSaraiva
Copy link
Contributor Author

Well, there are many tests that use MPI, these 4 tests are just the ones whom failed first since it stops with 5 failures. Maybe I can map all the ones that do use it, it will take some time though.

@Mindavi
Copy link
Contributor

Mindavi commented Apr 13, 2022

Some testing is always better than none.

@AtilaSaraiva AtilaSaraiva force-pushed the devito branch 2 times, most recently from 3525b60 to f7ae845 Compare April 22, 2022 19:09
@AtilaSaraiva
Copy link
Contributor Author

@SuperSandro2000 ping

@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 165484 run on x86_64-linux 1

2 packages failed to build and are new build failures:
6 packages built:
  • python310Packages.codepy
  • python310Packages.contexttimer
  • python310Packages.pyrevolve
  • python39Packages.codepy
  • python39Packages.contexttimer
  • python39Packages.pyrevolve

The following issues got detected with the above build packages.
Please fix at least the ones listed with your changed packages:

python310Packages.contexttimer:

A typo in pythonImportsCheck got detected.

python39Packages.contexttimer:

A typo in pythonImportsCheck got detected.

@AtilaSaraiva
Copy link
Contributor Author

Strange, it built successfully on my machine. It wil be kind of hard to reproduce the error.

@SuperSandro2000
Copy link
Member

Strange, it built successfully on my machine. It wil be kind of hard to reproduce the error.

nixpkgs review merges the branch into master. Try nixpkgs-review rev HEAD.

@AtilaSaraiva
Copy link
Contributor Author

I managed to reproduce the error, seems like a problem with sympy. I have zero ideas of how to solve this issue apart from forcing the usage of an older version of sympy through an overlay.

@Mindavi
Copy link
Contributor

Mindavi commented Apr 25, 2022

Maybe the release notes of sympy help? https://github.com/sympy/sympy/wiki/Release-Notes-for-1.10

@AtilaSaraiva AtilaSaraiva changed the title devito: init at 4.6.2 devito: init at unstable-2022-04-22 Apr 25, 2022
@AtilaSaraiva
Copy link
Contributor Author

@SuperSandro2000 , it seems like the problem was resolved with the most recent updates, so I changed the derivation version to the unstable format and also wrote an update script.

Comment on lines 40 to 66
def updateRequirementsPatch(requirements,derivation):
key = "distributed"
lineReq = findLine(key,requirements)[0]
lineDer = findLine(key,derivation)[0]
derivation[lineDer] = f""" --replace "{requirements[lineReq]}" "distributed" \ \n;"""

key = "flake8"
lineReq = findLine(key,requirements)[0]
lineDer = findLine(key,derivation)[0]
derivation[lineDer] = f""" --replace "{requirements[lineReq]}" "" \ \n;"""

key = "pytest"
lineReq = findLine(key,requirements)[0]
lineDer = findLine(key,derivation)[0]
derivation[lineDer] = f""" --replace "{requirements[lineReq]}" "" \ \n;"""

key = "pytest-runner"
lineReq = findLine(key,requirements)[0]
lineDer = findLine(key,derivation)[0]
derivation[lineDer] = f""" --replace "{requirements[lineReq]}" "" \ \n;"""

key = "pytest-cov"
lineReq = findLine(key,requirements)[0]
lineDer = findLine(key,derivation)[0]
derivation[lineDer] = f""" --replace "{requirements[lineReq]}" "" \ \n;"""

return derivation
Copy link
Member

@SuperSandro2000 SuperSandro2000 Apr 25, 2022

Choose a reason for hiding this comment

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

This could be easily replaced with a one sed script that removes everything after the first match of [<>]= and then we can scrap the entire updater script and use the unstable-updater.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be very nice to have an example haha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SuperSandro2000 I discussed this with some colleagues, and I find that the sed approach would be too easy to break, while my update script, although I admit is a bit overengineered, it is far more robust than a sed.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the script is very over engineered and as soon as it breaks the next person will most likely just remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SuperSandro2000 I adhered to your suggestions, pls do check if is ok now.

@AtilaSaraiva AtilaSaraiva force-pushed the devito branch 4 times, most recently from 39285da to 954a2d2 Compare April 26, 2022 13:14
@AtilaSaraiva AtilaSaraiva force-pushed the devito branch 2 times, most recently from a43e9f1 to 2f17522 Compare April 27, 2022 15:45
@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 165484 run on x86_64-linux 1

8 packages built:
  • python310Packages.codepy
  • python310Packages.contexttimer
  • python310Packages.devito
  • python310Packages.pyrevolve
  • python39Packages.codepy
  • python39Packages.contexttimer
  • python39Packages.devito
  • python39Packages.pyrevolve

@SuperSandro2000 SuperSandro2000 merged commit 33db599 into NixOS:master Apr 28, 2022
@AtilaSaraiva AtilaSaraiva deleted the devito branch April 28, 2022 14:40
@AtilaSaraiva
Copy link
Contributor Author

Thank you very much for your patience @SuperSandro2000 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: python 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants