Page MenuHomePhabricator

Create script to generate library
ClosedPublic

Authored by sashank on Sat, Aug 3, 2:27 AM.

Details

Reviewers
max
schrockn
alangenfeld
Group Reviewers
Restricted Project
Commits
R1:6c8085ab85af: Create script to generate library
Summary

Automate generating a dagster library. Uses a template found in bin/assets and a user-provided name to set up a library correctly, since it's easy to miss all the places you have to include the library name.

The generated library includes a simple "helloworld" resource and solid

Test Plan

To use:

python bin/create_library.py --name="sftp"

To test:

cd python_modules/libraries/dagster-sftp
pytest

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sashank created this revision.Sat, Aug 3, 2:27 AM
sashank edited the summary of this revision. (Show Details)Sat, Aug 3, 2:28 AM
sashank added inline comments.Sat, Aug 3, 2:38 AM
bin/assets/dagster-library-tmpl/dagster_library-tmpl/version.py
2

Any suggestions on how to correctly set this?

sashank updated this revision to Diff 3386.Sat, Aug 3, 2:43 AM

Change print statements

natekupp added inline comments.
bin/create_library.py
8

nit: copy_directory

natekupp added a subscriber: max.Sat, Aug 3, 3:32 AM

looks good to me! but will let @max take a look also before approving

alangenfeld added inline comments.
bin/assets/dagster-library-tmpl/dagster_library-tmpl/__init__.py.tmpl
4โ€“5

hm i wonder how likely these are to be useful vs just need to get deleted. Unclear to me if its better to generate more and delete or generate minimum

bin/assets/dagster-library-tmpl/dagster_library-tmpl/version.py
2

copy paste an existing version.py file

sashank updated this revision to Diff 3397.Mon, Aug 5, 4:46 PM

Fix nits

sashank marked an inline comment as done.Mon, Aug 5, 4:48 PM
sashank added a reviewer: max.Tue, Aug 6, 6:22 PM
alangenfeld resigned from this revision.Thu, Aug 8, 7:21 PM

i haven't added any libraries so defer to those who have

schrockn requested changes to this revision.Sun, Aug 11, 9:04 PM
schrockn added a subscriber: schrockn.

the resource, solid, and service stuff is too specific. not every library will need that. setup and folder structure is ๐Ÿ‘Œ

bin/assets/dagster-library-tmpl/README.md.tmpl
5

libraries are more general than an integration

bin/assets/dagster-library-tmpl/dagster_library-tmpl/resources.py.tmpl
1 โ†—(On Diff #3397)

imo this is too specific for a generic template

bin/assets/dagster-library-tmpl/dagster_library-tmpl_tests/test_resources.py.tmpl
1 โ†—(On Diff #3397)

yeah too much

bin/assets/dagster-library-tmpl/dagster_library-tmpl_tests/test_solids.py.tmpl
1 โ†—(On Diff #3397)

too much

This revision now requires changes to proceed.Sun, Aug 11, 9:04 PM
max added inline comments.Mon, Aug 12, 6:51 PM
bin/assets/dagster-library-tmpl/README.md.tmpl
5

+1

bin/assets/dagster-library-tmpl/dagster_library-tmpl/__init__.py.tmpl
4โ€“5

yeah, i would skip this and the resources.py / solids.py generation

bin/assets/dagster-library-tmpl/dagster_library-tmpl/version.py
2

could also use the bin/publish.py version machinery

4

this is trickier

bin/assets/dagster-library-tmpl/tox.ini.tmpl
11

i think omit this -- by convention we should force dev-reqs into the dagster file, this is a monorepo

bin/create_library.py
10

consider using .gitignore for this

sashank updated this revision to Diff 3626.Tue, Aug 13, 3:26 AM
  • Remove solid, resource, and tests for both
  • Use code from publish.py to set version number
  • Error if library with name already exists
sashank marked 7 inline comments as done.Tue, Aug 13, 3:27 AM
sashank updated this revision to Diff 3628.Tue, Aug 13, 3:56 AM

Rerun tests

sashank updated this revision to Diff 3645.Tue, Aug 13, 5:49 PM

Rerun tests

schrockn added inline comments.Tue, Aug 13, 5:50 PM
bin/create_library.py
33

nit" let's stick to single quotes

This revision is now accepted and ready to land.Tue, Aug 13, 5:50 PM
sashank updated this revision to Diff 3646.Tue, Aug 13, 5:54 PM

Change " -> '

sashank updated this revision to Diff 3647.Tue, Aug 13, 5:56 PM

Rerun tests

Harbormaster completed remote builds in B2918: Diff 3647.