-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(cli.project): enable optional minimalist configuration in templates #25245
feat(cli.project): enable optional minimalist configuration in templates #25245
Conversation
1495a52
to
75ca860
Compare
75ca860
to
7c75421
Compare
Note that excludes is flexible. dagster project scaffold --name qs |
To me no that would be even better. I assumed the comms to remove them would be involved. My baggage. |
Might be worth doing — something we'll look into!
…On Tue, Oct 15, 2024 at 12:36 PM, Daniel Bartley < ***@***.*** > wrote:
To me no that would be even better. I assumed the comms to remove them
would be involved. My baggage.
—
Reply to this email directly, view it on GitHub (
#25245 (comment) )
, or unsubscribe (
https://github.com/notifications/unsubscribe-auth/AAH7LZU4WTCTLD6GXANOKDDZ3VVEFAVCNFSM6AAAAABPZ5VJEKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJUHA2TEOJUGU
).
You are receiving this because your review was requested. Message ID: <dagster-io/dagster/pull/25245/c2414852945
@ github. com>
|
I'll make a new branch to remove those setup files completely. I still think excludes is valid to enable using the scaffold feature in the docs. Example the current docs suggest a reluctance to include the tests directory in hello_dagster. I'm pro aggressively remove setup files if you can handle it. |
companion PR to remove setup.cfg and setup.py from the templates as proposed by Pedram |
I think this looks great, @dbrtly - thanks! If you could address the pyright and ruff issues, then I believe this is good to merge. pyright:
ruff:
And it appears some the CLI unit tests have failed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above.
7c75421
to
360df7d
Compare
@dbrtly - I wasn't able to make a PR merging into this one, so it's merging into master. But my findings are that the And the wildcard, And I also had to remove the Feel free to make these changes here, and I can close out my PR. |
@cmpadden fwiw Colton, I'm ok with you committing directly to this branch. Not sure what was happening there. |
e916af9
to
2ec9386
Compare
Apologies for so many changes. Based on my experimental observation, there is no practical difference between these snippets: Given this PR adds the excludes flag, I took the liberty of deleting the duplicated code and deprecating the scaffold-code-location command. I tried to break the new code with this test but nothing weird happened. Only a mess.
|
No worries at all, @dbrtly. I pushed a couple of commits to fix ruff and pyright. However, we're still getting a few unit test failures. For a faster feedback loop, I'd suggest running it locally via: $ pytest -v dagster_tests/cli_tests/test_project_commands.py
|
1e96ccb
to
a0f13e9
Compare
got the tests passing. refactored some of them because flakey |
06d2320
to
8178a4e
Compare
91ecf1c
to
d3d97c5
Compare
PEP 621 (Nov 2020) introduced pyproject.toml. Setuptools is fully compatible with pyroject Completes dagster config in project template. Minimalist config to make adopting dagster easy.
d3d97c5
to
a12248f
Compare
I added the excludes validation step at the last minute. Fixed now.
|
Turns out this method was used elsewhere, and has caused some failures in other test suites, and our docs build, due to the renaming of this method:
Apologies that this one is taking so long to get over the finish line, but it turns out it's pretty intertwined with everything else! |
Where are the failing tests? |
Fixed flakey tests by refactor `os.path` to pathlib.Path.
f203e99
to
4340fa8
Compare
Looks to be used in
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're running green!
Thanks for sticking it out with this one, and thanks for your contribution.
PEP 621 (Nov 2020) introduced pyproject.toml.
Setuptools is fully compatible with pyroject.
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.
Summary & Motivation
See #25244
How I Tested These Changes
Last time I tried the run the test suite it failed because my macbook has an intel chipset.
Changelog
(CLI)
dagster project scaffold
: Add option to create dagster projects from templates with excluded files/filepathsdagster project scaffold-code-location
: Refactored to leverage scaffold command with excludes flag. Deprecated.