Skip to content

Make individual modules into separate packages#11

Open
anujkumar93 wants to merge 8 commits intosplit_ready_prefrom
split_packages_actual
Open

Make individual modules into separate packages#11
anujkumar93 wants to merge 8 commits intosplit_ready_prefrom
split_packages_actual

Conversation

@anujkumar93
Copy link
Copy Markdown
Owner

@anujkumar93 anujkumar93 commented Dec 3, 2018

@jtratner

This PR handles actually separating the repo into individual packages of stor, stor_s3, stor_swift, stor_dx.

Background

This is a continuation of (and maybe the end of) the story to split stor across 4 modules : stor, stor_dx, stor_swift, stor_s3. This is done to easy the dependencies that need to be installed to use a particular fact of stor, and to make it more modular. This PR is preceded by #10 for this story.

Changes

This PR handles the actual separation of modules into three different packages. The first commit just moves the files around, each module into their own nested package. The second commit is responsible for setting up the setup.py, setup.cfg, requirements.txt, etc for the individual packages, and changing makefile, tox.ini to get this separation into a working state. The key function find_cls_for_pth in init.py and its tests are implemented in this PR.
The rest of the commits address the review comments and bug/lint fixes.

Tested via:

The testing suite was run after adding tests for find_cls_for_pth (and a few other tests) successfully. The commands to the tests were changed here, but the logic remains similar.

TODOS

  • Decide how to make stor_swift, stor_s3, stor_dx as extras to stor, despite all of them being individual packages.

@jtratner
Copy link
Copy Markdown

jtratner commented Dec 4, 2018

from discussion: ignore b9b7708; next two are main commits; after that are smaller changes

Comment thread Makefile
$(WITH_VENV) pip install -r requirements-dev.txt --index-url=${PIP_INDEX_URL}
$(WITH_VENV) pip install -r requirements-docs.txt --index-url=${PIP_INDEX_URL}
$(WITH_VENV) pip install -r stor/requirements-setup.txt --index-url=${PIP_INDEX_URL}
$(WITH_VENV) ./run_all.sh 'pip install -e . --index-url=${PIP_INDEX_URL}' $(PACKAGE_NAMES)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

let's call ./run_all.sh ./within_each_folder_run.sh instead.

One point here (that is optional, but potentially nice way to learn Makefile-isms), would be that you could write this like we do for MSM:

install.%: venv
    $(WITH_VENV) cd $* && pip install -e . --index-url=${PIP_INDEX_URL}
clean.%:
    cd $* && python setup.py clean
    rm -rf $*/*.egg*
    rm -rf $*/__pycache__
develop.%: venv
    $(WITH_VENV) cd $* && python setup.py develop
develop: develop.stor develop.stor_dx develop.stor_swift develop.stor_s3

etc

Might be a little clearer to read and would not require second shell script.

Can also use patsubst to make it a little cleaner.

Totally optional to do so - but if you want excuse to understand makefiles a bit better could be good.

Comment thread Makefile
@@ -133,4 +134,4 @@ version:

.PHONY: fullname
fullname:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you prob only need this once for main repo, since names will stay all the same

Comment thread Makefile
.PHONY: dist
dist: venv fullname
$(WITH_VENV) python setup.py sdist
$(WITH_VENV) ./run_all.sh 'python setup.py sdist' $(PACKAGE_NAMES)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think our packaging systems need all of these to remain in the parent dist folder for upload to work, so you might want to edit that slightly.

Comment thread stor/stor/obs.py
can be any of s3 or swift.
The ``(drive)://`` prefix is required in the path.
"""
if hasattr(self, 'path_converted') and self.path_converted \
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why did this sneak in here?

Comment thread stor_dx/requirements.txt
@@ -0,0 +1,4 @@
cached-property
contextlib2
dxpy>=0.265.0; python_version <= '2.7'
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

note: remember this needs to just be dxpy

Comment thread stor/setup.py
@@ -0,0 +1,6 @@
from setuptools import setup
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this one may need to have an install_requires== to specific versions of required sub-packages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants