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

Deprecate odm2 #147

Merged
merged 4 commits into from
Feb 27, 2018
Merged

Deprecate odm2 #147

merged 4 commits into from
Feb 27, 2018

Conversation

lsetiawan
Copy link
Member

Overview

This is an alternate to #145

@lsetiawan
Copy link
Member Author

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

@lsetiawan
Copy link
Member Author

@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
Copy link
Member

emiliom commented Feb 16, 2018

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
Copy link
Member

emiliom commented Feb 17, 2018

Addresses #146

@ocefpaf
Copy link
Member

ocefpaf commented Feb 17, 2018

@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 emiliom requested review from kwuz and removed request for kwuz February 18, 2018 03:23
@emiliom
Copy link
Member

emiliom commented Feb 20, 2018

@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
Copy link
Member Author

lsetiawan commented Feb 21, 2018

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
Copy link
Member

emiliom commented Feb 26, 2018

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 😢. Any ideas?

@lsetiawan
Copy link
Member Author

@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
Copy link
Member

emiliom commented Feb 27, 2018

@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 😼)

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
Copy link
Member

ocefpaf commented Feb 27, 2018

@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
Copy link
Member Author

lsetiawan commented Feb 27, 2018

@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
Copy link
Member Author

What do you think of sys.stdout?

@ocefpaf
Copy link
Member

ocefpaf commented Feb 27, 2018

@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
Copy link
Member

emiliom commented Feb 27, 2018

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
Copy link
Member Author

All works, got @emiliom blessing 😉 Merging! 😃

@lsetiawan lsetiawan merged commit 981f4c6 into ODM2:development Feb 27, 2018
@lsetiawan lsetiawan deleted the deprecate_odm2 branch February 27, 2018 17:15
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.

3 participants