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.

Sign up for GitHub

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

Jump to bottom

fastapi-dls: init #358647

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fastapi-dls: init #358647

wants to merge 2 commits into from

Conversation

MakiseKurisu
Copy link
Contributor

@MakiseKurisu MakiseKurisu commented Nov 24, 2024 •
edited
Loading

Add fastapi-dls and associated NixOS module.

fastapi-dls is an open source implementation of NVIDIA Delegated License Service (DLS).

I'm dogfooding this PR on my own machine currently.

Project homepage: https://git.collinwebdesigns.de/oscar.krause/fastapi-dls

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Nov 24, 2024
@MakiseKurisu MakiseKurisu force-pushed the fastapi-dls branch 2 times, most recently from 352e7ab to 6e44fa0 Compare November 24, 2024 08:52
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Nov 24, 2024
GaetanLepage
GaetanLepage requested changes Nov 26, 2024
View reviewed changes
pkgs/by-name/fa/fastapi-dls/package.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/fastapi-dls.nix Outdated Show resolved Hide resolved
@h7x4 h7x4 added the 8.has: module (new) This PR adds a module in `nixos/` label Nov 27, 2024
@MakiseKurisu MakiseKurisu removed the 8.has: module (update) This PR changes an existing module in `nixos/` label Nov 27, 2024
@MakiseKurisu MakiseKurisu marked this pull request as draft December 7, 2024 07:04
@MakiseKurisu
Copy link
Contributor Author

Just found some issue with license acquisition.

@github-actions github-actions bot added the 8.has: module (update) This PR changes an existing module in `nixos/` label Dec 8, 2024
@MakiseKurisu
Copy link
Contributor Author

I'm currently getting 400 error when running it on NixOS:

INFO:     192.168.9.21:0 - "POST /auth/v1/code HTTP/1.1" 200 OK
INFO:     192.168.9.21:0 - "POST /auth/v1/token HTTP/1.1" 400 Bad Request

I tried various method to dump the bad request body but none of them worked. At this point I'm stuck.
Some logs can be found on the Discord: https://discord.com/channels/829786927829745685/830510800614719539/1314851346859429909

@MakiseKurisu MakiseKurisu marked this pull request as ready for review December 10, 2024 14:32
@MakiseKurisu
Copy link
Contributor Author

@mrzenc helped me with the issue, and his patch is now included in the PR. If you can leave a review that'd very helpful.

The service is working again on my computer.

GaetanLepage
GaetanLepage reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

The diff looks good to me. However, I lack experience on NixOS module reviewing and would advise for another committer to take a look.

SuperSandro2000
SuperSandro2000 reviewed Dec 11, 2024
View reviewed changes
nixos/modules/services/misc/fastapi-dls.nix Outdated
##### implementation
config =
let
envFile = pkgs.writeText "fastapi-dls.env" ''
Copy link
Member

Choose a reason for hiding this comment

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

This is not extensible. Also we should probably strip the commented things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The content (including comments) here is based on the upstream env file and is how the upstream configure the service.

These are just environment variables though, so we can definitely use different implementation. Do you have an example in mind?

Copy link

Choose a reason for hiding this comment

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

systemd.services.fastapi-dls.environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to systemd.services.fastapi-dls.environment but I'm not sure how this is more extensible.

nixos/modules/services/misc/fastapi-dls.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/fastapi-dls.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/fastapi-dls.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/fastapi-dls.nix

listenPort = lib.mkOption {
type = lib.types.port;
default = 443;
Copy link
Member

Choose a reason for hiding this comment

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

That is a bad default. We should use something like 5000 or whatever upstream uses except 443

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upstream does use 443 though:
https://git.collinwebdesigns.de/oscar.krause/fastapi-dls/-/blob/main/.DEBIAN/env.default?ref_type=heads#L6
https://git.collinwebdesigns.de/oscar.krause/fastapi-dls/-/blob/main/Dockerfile?ref_type=heads#L21

nixos/modules/services/misc/fastapi-dls.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/fastapi-dls.nix Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/fetchgit-hub-add-tag-argument/56710/5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrzenc mrzenc mrzenc left review comments

@GaetanLepage GaetanLepage GaetanLepage requested changes

@pluiedev pluiedev Awaiting requested review from pluiedev

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MakiseKurisu @nixos-discourse @SuperSandro2000 @GaetanLepage @mrzenc @h7x4