Page MenuHomePhabricator

Create script to generate library
ClosedPublic

Authored by sashank on Aug 3 2019, 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
Branch
create-library-script
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

sashank created this revision.Aug 3 2019, 2:27 AM
sashank edited the summary of this revision. (Show Details)Aug 3 2019, 2:28 AM
sashank added inline comments.Aug 3 2019, 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.Aug 3 2019, 2:43 AM

Change print statements

natekupp added inline comments.
bin/create_library.py
8

nit: copy_directory

natekupp added a subscriber: max.Aug 3 2019, 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.Aug 5 2019, 4:46 PM

Fix nits

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

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

schrockn requested changes to this revision.Aug 11 2019, 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.Aug 11 2019, 9:04 PM
max added inline comments.Aug 12 2019, 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.Aug 13 2019, 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.Aug 13 2019, 3:27 AM
sashank updated this revision to Diff 3628.Aug 13 2019, 3:56 AM

Rerun tests

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

Rerun tests

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

nit" let's stick to single quotes

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

Change " -> '

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

Rerun tests

Harbormaster completed remote builds in B2918: Diff 3647.