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

Recover app bootstrapping in Python entrypoints #5152

Conversation

helderco
Copy link
Contributor

Follow-up to #5060

Python had initial support for entrypoints but because of publishing to Conda, since we didn't publish the extensions code, it was easier to just remove the entrypoint executable from the SDK (in #4399, sdk/python/pyproject.toml):

- [tool.poetry.scripts]
- # FIXME: uncomment when extensions become available
- # dagger-server-py = "dagger.server.cli:app"
- dagger-py = "dagger.cli:app"

This PR recovers __main__.py to enable calling with python -m, without creating the dagger-server-py entrypoint. That executable did allow replacing the entrypoint shell wrapper in the runtime image, apart from needing cd to change the workdir when running the runtime entrypoint. But it also installed it in users venvs, that's why I've not recovered it.

I also simplified installing via requirements.txt. For now we can require it.

In the demo, I've changed the dependency to install SDK library from local dir. It'll require a release for dagger-io[sever] to have the __main__.py module, but it'll also allow iterating on the Python DX going forward.

Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
@helderco helderco requested a review from sipsma May 18, 2023 08:50
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fyi: This module is used for codegen:

"python -m dagger generate --output ${GEN_PATH}/gen.py",
"python -m dagger generate --output ${GEN_PATH}/gen_sync.py --sync",


from .cli import app

sys.exit(app(prog_name="dagger-server-py"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fyi: the dagger-server-py name here isn't important. It's just to replace the arg[0] for the command when showing help.

set -exu
# go to the workdir
cd %q
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This cd is only needed because at runtime the workdir needs to be where ConfigPath is, not where dagger do is run from.

llb.Scratch().
File(llb.Copy(contextState, p.Directory.Dir, "/src")),
}).
// FIXME(samalba): Install python dependencies not as root
// FIXME(samalba): errors while installing requirements.txt will be ignored because of the `|| true`. Need to find a better way.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's many dependency managers in Python. Also in Node.js there's not only npm, there's pnpm, yarn, etc... We either need to add code to support detecting which one the user is using (limiting to a few most popular), or just support one. For now, let's just require requirements.txt here.

@@ -1 +1 @@
dagger-io
dagger-io[server]
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 [server] extra is needed to install Strawberry. It's configured here:

strawberry-graphql = {version = ">=0.133.5", optional = true}
[tool.poetry.extras]
server = ["strawberry-graphql"]

Copy link
Member

@gerhard gerhard left a comment

Choose a reason for hiding this comment

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

LGTM 🐿

@gerhard gerhard merged commit 03ee4e4 into dagger:main May 18, 2023
16 checks passed
@helderco helderco deleted the helder/dag-1504-recover-app-bootstrapping-in-python branch May 18, 2023 12:18
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.

None yet

2 participants