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

Add support for loading multiple modules from pyproject.toml #12551

Conversation

bendnorman
Copy link

Summary & Motivation

It would be nice to load multiple modules using pyproject.toml so users don't have to type out all the modules they want to load using the CLI command. See issue #11439 and discussion #11167.

How I Tested These Changes

I added a new unit test in dagster_tests/cli_tests/test_toml_loading.py

@vercel
Copy link

vercel bot commented Feb 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
dagit-storybook ⬜️ Ignored (Inspect) Feb 25, 2023 at 5:04PM (UTC)

@vercel
Copy link

vercel bot commented Feb 25, 2023

@bendnorman is attempting to deploy a commit to the Elementl Team on Vercel.

A member of the Team first needs to authorize it.

@erinkcochran87 erinkcochran87 requested review from gibsondan and removed request for erinkcochran87 February 27, 2023 15:06
@gibsondan gibsondan requested a review from schrockn March 2, 2023 20:49
@gibsondan
Copy link
Member

This seems totally reasonable to me. @schrockn do you feel OK about the syntax here? The alternative would be to have separate 'module_name' (when its singular) and 'module_names' (when it's plural) but a single key is probably better for consistency.

@schrockn
Copy link
Member

schrockn commented Mar 8, 2023

My bad on the tardiness to the review here.

One alternative to consider here is a different more "advanced" entry to allow for future extensions.

modules = [{module_name: foo}, {module_name: bar}]

and also also the specification of attribute and location_name here.

The most generic and future-proof being:
'code_locations = [{type:module, module_name: foo}, {type:module, module_name: bar}]`

My recommendation is a new modules toml entry.

Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

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

Back to you for any thoughts about incorporating @schrockn 's suggestion. Thanks again for sending this out!

@schrockn schrockn removed their request for review February 20, 2024 17:48
schrockn pushed a commit that referenced this pull request Sep 13, 2024
## Summary & Motivation

> Copied the same motivation that was stated back here, where it wasn't
proceeded further #12551

It would be nice to load multiple modules using pyproject.toml so users
don't have to type out all the modules they want to load using the CLI
command. See issue #11439
and discussion #11167.

I tried to add the recommendations mentioned here:
#12551 (comment)

## How I Tested These Changes
I added a new unit test in dagster_tests/cli_tests/test_toml_loading.py


## Changes:
### Add: Support for loading multiple modules from `pyproject.toml` in
Dagster (#4bad886)

-------------------------------------------------------------------------------------
* Updated documentation to include example of loading multiple modules
(`DagsterDevTabs.mdx`)
* Added `is_valid_modules_list` function to validate module lists in
`pyproject.toml` (`load_target.py`)
* Modified `get_origins_from_toml` to support loading multiple modules
from `pyproject.toml` (`load_target.py`)
* Added new test case `test_load_multiple_modules_from_toml` to verify
loading multiple modules (`test_toml_loading.py`)
* Created a new TOML test file `multiple_modules.toml` to simulate
loading multiple modules (`toml_tests/multiple_modules.toml`)
@graphite-app graphite-app bot added the area: docs Related to documentation in general label Sep 28, 2024
@graphite-app graphite-app bot requested a review from PedramNavid September 28, 2024 20:06
Copy link

graphite-app bot commented Sep 28, 2024

Graphite Automations

"Label and add CE on all Docs" took an action on this PR • (09/28/24)

3 reviewers were added and 1 label was added to this PR based on Pedram Navid's automation.

@PedramNavid
Copy link
Contributor

Closing stale PR.

@PedramNavid PedramNavid closed this Oct 1, 2024
@bendnorman
Copy link
Author

This was resolved by #22327 👍

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.

4 participants