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

Deal with, document dependencies issue, add first README.md #22

Merged
merged 13 commits into from
Feb 26, 2020

Conversation

vinceatbluelabs
Copy link
Contributor

@vinceatbluelabs vinceatbluelabs commented Feb 25, 2020

Changes:

  • Add README.md. I'm sure to have more updates on this later on, but I wanted to put something in place now that we can start from.
  • Add INSTALL.md documenting complexities around pyarrow, psycopg2 and pandas
  • Move psycopg2 to mover-cli extra only, but now in -binary form. See INSTALL.md for the rationale.
  • Deals with a transitive dependency issue with awscli and PyYAML that we've been trying to ignore and had worked around internally. I tested this fix with a fresh pyenv and our README.md instructions with pip install seem to work now.

@vinceatbluelabs vinceatbluelabs changed the title Add README.md, INSTALL.md and move psycopg2 to mover-cli extra Add README.md, INSTALL.md and move psycopg2 to mover-cli extra, but now in -binary form Feb 25, 2020
@@ -24,7 +24,7 @@ commands:
. venv/bin/activate
# venv/ dir doesn't seem to save enough info to keep the
# editable installation
pip install --progress-bar=off -e .
pip install --progress-bar=off -e '.[movercli]'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! This wasn't the same as the pip install below and it should have been. This may speed up our tests a touch.

export CFLAGS
LDFLAGS="-L$(brew --prefix openssl)/lib"
export LDFLAGS
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this and this is no longer needed with psycopg2-binary!

@vinceatbluelabs vinceatbluelabs changed the title Add README.md, INSTALL.md and move psycopg2 to mover-cli extra, but now in -binary form Deal with, document dependencies issue, add first README.md Feb 25, 2020
@vinceatbluelabs vinceatbluelabs marked this pull request as ready for review February 25, 2020 23:23
@@ -39,8 +39,19 @@
},
install_requires=[
'boto>=2,<3', 'boto3',
'jsonschema', 'timeout_decorator', 'awscli',
'PyYAML', 'psycopg2',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See notes in INSTALL.md for why I think dropping psycopg2 here is the right change.

Copy link
Contributor

@cwegrzyn cwegrzyn left a comment

Choose a reason for hiding this comment

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

A few small typos I noticed, but generally LGTM :shipit:


### pyarrow

`pyarrow` is a Python wrapper around the Apache Arrow native library.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... is it worth making a mover-cli target that doesn’t include this? I guess it might depend on how hard it is to install these libraries in general. Should we maybe include steps for homebrew and whatever Linux distro we have already figured out for CI/docker?

Copy link
Contributor Author

@vinceatbluelabs vinceatbluelabs Feb 26, 2020

Choose a reason for hiding this comment

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

Hmm... is it worth making a mover-cli target that doesn’t include this

No objections here.

Should we maybe include steps for homebrew and whatever Linux distro we have already figured out for CI/docker?

That'd be great! I don't have any off-the-shelf to drop in. Same feeling on not holding up this PR, but if I wasn't super clear about it, having spaces for those sorts of instructions are exactly why I added this file.

INSTALL.md Outdated
for local Parquet manipulation).
* `pip3 install records-mover[gsheets]` - Minimal install plus API
libraries to access Google Sheets.
* `pip3 install records-mover[mover-cli]` - Install everything and
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it mover-cli or movercli? I’ve seen it both ways in the docs (will flag)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

movercli. Where else did you see mover-cli in the docs?

INSTALL.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated

# This is a SQLAlchemy database engine.
#
# You can instead call job_context.get_db_engine('cred name').
Copy link
Contributor

Choose a reason for hiding this comment

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

Some old uses of job_context in here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed; that's all that grep found.

@vinceatbluelabs vinceatbluelabs merged commit b1997bb into master Feb 26, 2020
@vinceatbluelabs vinceatbluelabs deleted the README branch February 26, 2020 15:00
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