-
Notifications
You must be signed in to change notification settings - Fork 157
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
switch nix recipe to flake #992
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Changes required as running the notebook live and running notebook with nbval resulted in different outputs.
Where is the documentation if the PR is from a fork? |
In an artifact of the build, but not hosted anywhere |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Azure stalled, probably when @wd15 closed and reopened this PR. Attempting to rerun the CI says: ``` Encountered error(s) while parsing pipeline YAML: Could not get the latest source version for repository usnistgov/fipy hosted on https://github.com/ using ref refs/pull/993/merge. GitHub reported the error, "No commit found for SHA: refs/pull/993/merge" ``` Hopefully this commit will trigger a fresh build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests passed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM. Small question about maintenance overhead and hard-coding package versions.
fipy/tools/logging/environment.py
Outdated
stdout, _ = p.communicate() | ||
stdout = stdout.decode('ascii') | ||
|
||
info["nix_info"] = json.loads(stdout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably, this should just return json.loads(stdout)
, rather than nesting it in a dict. conda_info()
is a little different because it collects the results of both conda info
and conda env export
. One option is that each of these routines should return a dict containing one or more info["???_info"] and then have fipy.__init__.py
call _fipy_environment.update(environment.???_info())
on each of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let you decide on that.
Update the Nix recipe as Nix test previously failing.