ODM2 / ODM2PythonAPI

A set of Python functions that provides data read/write access to an ODM2 database by leveraging SQLAlchemy.
http://odm2.github.io/ODM2PythonAPI/
BSD 3-Clause "New" or "Revised" License
4 stars 13 forks source link

Deprecate odm2 #147

Closed lsetiawan closed 6 years ago

lsetiawan commented 6 years ago

Overview

This is an alternate to #145

lsetiawan commented 6 years ago

@emiliom I guess the individual commits diff views are showing the correct changes, but the overall diff is showing same changes as #145 😄

lsetiawan commented 6 years ago

@ocefpaf Is there a way to ignore prints within code so that travis ci is not complaining or do I have to put # noqa on every single print()?

emiliom commented 6 years ago

I guess the individual commits diff views are showing the correct changes, but the overall diff is showing same changes as #145

I don't follow. But maybe I should just call you to ask you about it ...

... or do I have to put # noqa on every single print()?

Is that what those are?! I'd seen them before somewhere and thought they were plain old comments.

emiliom commented 6 years ago

Addresses #146

ocefpaf commented 6 years ago

@ocefpaf Is there a way to ignore prints within code so that travis ci is not complaining or do I have to put # noqa on every single print()?

We can drop flake8-print if you do want to use print in the code.

emiliom commented 6 years ago

@lsetiawan I tried to create a local conda env based on your deprecate_odm2 branch to test your PR (mostly to test that both the new and old ways to reference module import paths work). But import odm2api failed. What am I doing wrong? Here's how I created the environment:

# I downloaded requirements.txt and requirements-dev.txt from your fork (I'd already done a git clone)
conda create --name odm2api_dev python=2.7 --file requirements.txt --file requirements-dev.txt -c odm2
source activate odm2api_dev
pip install git+https://github.com/lsetiawan/ODM2PythonAPI.git@deprecate_odm2

Thanks for your help!

lsetiawan commented 6 years ago

I downloaded requirements.txt and requirements-dev.txt from your fork (I'd already done a git clone)

If you already done a git clone and you have installed the dependencies, you can just cd into the repository and do pip install -e . but make sure that you are on the correct branch of ODM2PythonAPI. Thanks.

emiliom commented 6 years ago

I downloaded requirements.txt and requirements-dev.txt from your fork (I'd already done a git clone)

If you already done a git clone and you have installed the dependencies, you can just cd into the repository and do pip install -e . but make sure that you are on the correct branch of ODM2PythonAPI. Thanks.

I tried this (pip install -e .), with the same result as before. odm2api is not recognized when I try to import it during an ipython or jupyter notebook session. But then I looked into it a bit more, and something strange is going on.

When I start ipython, a very old version of ipython is opened (4.2.0), and the Python version itself is very old (2.7.12, from July 2016). But conda list doesn't shown these old versions, and when I open a python session (not ipython), the Python version is the current one (2.7.14). It looks as if my conda "root" is interfering -- those are the ipython and python versions on my conda root.

Then I tried installing the latest odm2api from conda (odm2 channel) the normal way, just to double check:

conda create -n odm2api_test1 -c odm2 python=2.7 jupyter odm2api

Everything was fine. The up-to-date ipython and python versions are installed, and no problem importing odm2api.

So, I don't know what's going on, but I'm not able to test your branch :cry:. Any ideas?

lsetiawan commented 6 years ago

@emiliom You're having trouble because your ipython is probably using the root ipython environment. #149 adds ipython for development that way it's using the environment ipython. You can check this by executing which ipython to check where it's from. Here are the steps to install the environment:

git clone https://github.com/ODM2/ODM2PythonAPI.git
cd ODM2PythonAPI/
conda create --name odm2api_dev -c conda-forge python=2.7 ipython --file requirements.txt --file requirements-dev.txt 
source activate odm2api_dev
pip install -e .
emiliom commented 6 years ago

@lsetiawan: Alright! I finally created a local conda dev environment with a good ipython and jupyter, then tested this using an old notebook that examines the LBR SQLite DB. It worked as expected both without the deprecated ODM2 package hierarchy, and with it (which in turn produced the obnoxious deprecation warnings, as planned :smirk_cat:)

I also reviewed the changes to the modules, and everything looks good (nice strategy, BTW!).

But given that TravisCI is failing and I don't know enough to figure out why (ie, to see if it's those print statements you referred to earlier), I'll let you merge the PR.

ocefpaf commented 6 years ago

@lsetiawan if you are going to use print as a mechanism to output to stdout you can remove flake8-print from https://github.com/ODM2/ODM2PythonAPI/blob/master/requirements-dev.txt#L7 and the tests will pass.

lsetiawan commented 6 years ago

@lsetiawan if you are going to use print as a mechanism to output to stdout you can remove flake8-print

@ocefpaf, What do you suggest is a better mechanism to output to stdout? log? Thanks!

lsetiawan commented 6 years ago

What do you think of sys.stdout?

ocefpaf commented 6 years ago

@ocefpaf, What do you suggest is a better mechanism to output to stdout? log? Thanks!

I would not say better b/c I don't really know the differences and possible issues that arise from using a simple print. Something like logging.StreamHandler(sys.stdout) is the recommended "professional" way to do that.

With that said. I am 100% fine to just accept the print statements and remove flake8-print.

emiliom commented 6 years ago

I think we should stick with accepting the print statements. If that's been a common practice in odm2api, I don't think it's our role to place restrictions on it via a side mechanism (flake8-print), as opposed to an actual, direct discussion.

Besides, this PR has nothing to do with that issue. Let's move forward with this PR.

lsetiawan commented 6 years ago

All works, got @emiliom blessing :wink: Merging! :smiley: