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

openai: add python3Packages.wandb dependency #161725

Merged
merged 1 commit into from
Mar 21, 2022

Conversation

malob
Copy link
Member

@malob malob commented Feb 24, 2022

Motivation for this change

The openai executable from python3Packages.openai added new functionality that depends on python3Packages.wandb. This PR adds that dependency, and also fixes some build issues with packages wandb depends on.

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.

@malob
Copy link
Member Author

malob commented Feb 24, 2022

Looks like the fixes to the dependencies of wandb will require rebuilding over a thousand packages!!!

I'm running nixpkgs-review on my systems, but it's going to take a while :)

@malob
Copy link
Member Author

malob commented Feb 24, 2022

The @ofborg failure on Linux seems to be due to python3Packages.sanic (which is a dependency of wandb failing to build. I'm looking into it.

Copy link
Member

@gador gador left a comment

Choose a reason for hiding this comment

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

The mass rebuild is due to changes in pkgs/development/python-modules/dnspython/default.nix
Many python packages depend on this package.

Do you need to change it for openai to work? If so: Please add the changes to dnspython in a seperate PR against staging.

pkgs/development/python-modules/dnspython/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/dnspython/default.nix Outdated Show resolved Hide resolved
@malob
Copy link
Member Author

malob commented Feb 24, 2022

Do you need to change it for openai to work? If so: Please add the changes to dnspython in a seperate PR against staging.

@gador, dnspython is a dependency of wandb. wandb isn't required for openai to build, but some functionality of the executable isn't available without it, so I'd like to add it.

Just to confirm, you'd like me to make a new PR with the changes to dnspython, that targets staging, then come back to this PR with that change removed once the new PR lands?

@malob
Copy link
Member Author

malob commented Feb 24, 2022

Blocked on: #161740, #159516, and #162257 landing on master.

@gador
Copy link
Member

gador commented Feb 25, 2022

I'd think that's a sensible action, yes.

This way, this PR gets reviewed and merged faster and mass rebuilds (due to dnspython) should go to staging (and will take some time to get merged)

@malob
Copy link
Member Author

malob commented Mar 21, 2022

Now only blocked on #161740 hitting master.

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

LGTM

Result of nixpkgs-review pr 161725 run on x86_64-linux 1

1 package built:
  • openai (python39Packages.openai)

@jonringer jonringer merged commit 9d237af into NixOS:master Mar 21, 2022
@jonringer
Copy link
Contributor

to rebase to staging:

# finding the common merge base will avoid pinging all CODEOWNERs
common=$(git merge-base origin/master origin/staging)
git rebase --onto=$common HEAD~1 # or however many commits you want to pull
git push .. ... --force

then change the base branch in the github PR from master -> staging

See https://nixos.org/nixpkgs/manual/#submitting-changes-staging-branch for more details on staging branch

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

Successfully merging this pull request may close these issues.

3 participants