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

refactor: consolidate config to pyproject.toml only #25303

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

Conversation

dbrtly
Copy link
Contributor

@dbrtly dbrtly commented Oct 16, 2024

Summary & Motivation

[PEP 621 (Nov 2020) introduced pyproject.toml(https://peps.python.org/pep-0621/).
Setuptools is fully compatible with pyproject.toml

All the config in setup.cfg and setup.py will also be in pyproject.toml project template if this is merged.
Minimalist config to make adopting dagster easy. Fewer files: ^^ elegance.

Pedram encouraged the more aggressive refactoring here. #25245

How I Tested These Changes

Last time I tried the run the test suite it failed because my macbook has an intel chipset.

Changelog

Consolidate configuration in dagster projects created via the dagster project scaffold command to pyproject.toml.

Copy link
Contributor

@PedramNavid PedramNavid left a comment

Choose a reason for hiding this comment

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

I'm very supportive, but I want to call in a few bigger guns for their thoughts. Maybe @alangenfeld and @yuhan?

I think we might need to do a pass on our docs as well before we can merge this:

getting-started/create-new-project.mdx
36:The `dagster project scaffold` command generates a folder structure with a single Dagster code location and other files, such as `pyproject.toml` and `setup.py`. This takes care of setting things up with an empty project, enabling you to quickly get started.
104:You can specify new Python dependencies in `setup.py`.

guides/dagster/recommended-project-structure.mdx
76:├── setup.py
165:So far, we've discussed our recommendations for structuring a large project which contains only one code location. Dagster also allows you to structure a project with multiple definitions. We don't recommend over-abstracting too early; in most cases, one code location should be sufficient. A helpful pattern uses multiple code locations to separate conflicting dependencies, where each definition has its own package requirements (e.g., `setup.py`) and deployment specs (e.g., Dockerfile).

dagster-plus/deployment/serverless.mdx
87:  <code>setup.py</code> or <code>requirements.txt</code> file.
89:For example, in `my-dagster-project/setup.py`:
126:Any dependencies specified in either `requirements.txt` or `setup.py` will be installed for you automatically by the Dagster+ Serverless infrastructure.
136:To add data files to your deployment, use the [Data Files Support](https://setuptools.pypa.io/en/latest/userguide/datafiles.html) built into Python's `setup.py`. This requires adding a `package_data` or `include_package_data` keyword in the call to `setup()` in `setup.py`. For example, given this directory structure:
138:    - setup.py
146:If you want to include the `data` folder, modify your `setup.py` to add the `package_data` line:
149:# setup.py
196:As far as possible, add all dependencies by including the corresponding native Python bindings in your `setup.py`. When that is not possible, you can build and upload a custom base image that will be used to run your Python code.

integrations/dbt/quickstart.mdx
138:**Note**: This example assumes that your existing Dagster project includes both `assets.py` and `definitions.py` files, among other required files like `setup.py` and `pyproject.toml`. For example, your project might look like this:
147:└── setup.py

integrations/dbt-cloud.mdx
183:      - uses: actions/setup-python@v2

integrations/dbt/using-dbt-with-dagster-plus/hybrid.mdx
54:       pip install . --upgrade --upgrade-strategy eager                                            ## Install the Python dependencies from the setup.py file, ex: dbt-core and dbt-duckdb
61:   - **Add any [adapters](https://docs.getdbt.com/docs/connect-adapters) and libraries used by dbt to your `setup.py` file**.

integrations/dbt/using-dbt-with-dagster-plus/serverless.mdx
121:│   ├── setup.py
171:└── setup.py
202:       pip install . --upgrade --upgrade-strategy eager                                            ## Install the Python dependencies from the setup.py file, ex: dbt-core and dbt-duckdb
209:   - **Add any [adapters](https://docs.getdbt.com/docs/connect-adapters) and libraries used by dbt to your `setup.py` file**. In this example, we're using `dbt-core` and `dbt-duckdb`.

guides/understanding-dagster-project-files.mdx
31:├── setup.py
153:        and meant to replace <code>setup.py</code>, but we may still include a{" "}
154:        <code>setup.py</code> for compatibility with tools that do not use this
159:      <td>setup.py</td>
170:        An ini file that contains option defaults for <code>setup.py</code>{" "}
307:├── setup.py
329:├── setup.py
356:├── setup.py
384:├── setup.py
408:├── setup.py

dagster-plus/getting-started.mdx
287:- **If not using the template**, add `dagster-cloud` as a dependency in `setup.py`. [Click here for an example](https://github.com/dagster-io/quickstart-etl/blob/main/setup.py). This is already done for you if using the starter kit.
425:   The Docker image should contain a Python environment with `dagster`, `dagster-cloud`, and your code. For reference, see the [example Dockerfile](https://github.com/dagster-io/dagster-cloud-hybrid-quickstart/blob/main/Dockerfile) in our quickstart repository. The example uses `pip install .` to install the code including the dependencies specified in [`setup.py`](https://github.com/dagster-io/dagster-cloud-hybrid-quickstart/blob/main/setup.py).

dagster-plus/managing-deployments/dagster-cloud-yaml.mdx
103:└── setup.py
261:└── setup.py
291:- `directory` - Setting a build directory is useful if your `setup.py` or `requirements.txt` is in a subdirectory instead of the project root. This is common if you have multiple Python modules within a single Dagster project.

@dbrtly dbrtly force-pushed the scaffold-pyproject branch from 8244048 to b27489c Compare October 18, 2024 13:33
@graphite-app graphite-app bot added the area: docs Related to documentation in general label Oct 18, 2024
@dbrtly
Copy link
Contributor Author

dbrtly commented Oct 18, 2024

I did a test on dagster+. Turns out dagster+ has a hardcoded dependency on setup.py or requirement.txt.

Screenshot 2024-10-19 at 07 52 43 Screenshot 2024-10-19 at 08 09 46

The responsible code is here. Not sure if it is justifiably pessimistic about file dependency.
https://github.com/dagster-io/dagster-cloud/blob/main/dagster-cloud-cli/dagster_cloud_cli/commands/serverless/__init__.py#L534-L545

@dbrtly
Copy link
Contributor Author

dbrtly commented Oct 19, 2024

@PedramNavid @cmpadden
How do you feel about installing on dagster+ with uv to replace pex?
https://docs.astral.sh/uv/concepts/tools/

dagster-io/dagster-cloud-action#202

After above PR, I will do PR on dagster-cloud to remove:
https://github.com/dagster-io/dagster-cloud/blob/main/dagster-cloud-cli/dagster_cloud_cli/commands/serverless/__init__.py#L534-L545
and extend
https://github.com/dagster-io/dagster-cloud/blob/main/dagster-cloud-cli/dagster_cloud_cli/commands/serverless/Dockerfile#L6

I'd like to split the build code from the deploy code. It feels like the github-action it could be simpler with stricter separation of build/push-to-registry/deploy.

@dbrtly dbrtly force-pushed the scaffold-pyproject branch from c4dac17 to d2ec720 Compare October 19, 2024 10:11
@cmpadden cmpadden requested review from alangenfeld and yuhan October 29, 2024 18:02
@alangenfeld alangenfeld requested review from smackesey and removed request for alangenfeld October 31, 2024 15:18
@cmpadden cmpadden requested a review from mlarose November 22, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: docs Related to documentation in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants