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

DONE: Functionality for dev-build #3

Merged
merged 15 commits into from
Jul 12, 2020

Conversation

Smat26
Copy link
Contributor

@Smat26 Smat26 commented Mar 17, 2020

PR to add dev-build flag:

  • to allow git clone and
  • pip --edit install of modules.

Changes include:

  • Ability to change working directory before executing command
  • Removed unused imports
  • Re-ordered precedence (pyenv before webfe)
  • Added list of git repos required for setup.

Dev-tested on pdbe-ondep-test machine

@Smat26 Smat26 changed the title WIP: Functionality for dev-build DONE: Functionality for dev-build Mar 17, 2020
@berrisfordjohn
Copy link
Contributor

looks good to me. Has been used to setup Ahsan's machine at PDBe

@epeisach
Copy link
Contributor

Why are you installing scipy in the update script - and hardcoding a specific version? Is there a dependency that is broken? If so - that should be fixed - and comments around the code explaining the reason for doing this.

Version 1.2.3 is available for python 2. For python 3, we should move to the 1.4 series. Is there a reason for 1.2.x - there are ways to code that without coding a specific version - and making it depend on python 2 only.

If there is a reason not to trust dependencies or installing dependencies with our extra pip options messes things up, base_package/requirements.txt is the place - and you can see python version specific logic.

@epeisach
Copy link
Contributor

scipy is pulled in by wwpdb-TEMPy for python2 which is part of the base install.

There is also an assumption of what directory you are sitting in.

open('../base_packages/requirements_wwpdb_dependencies.txt')
I do not see a chdir before this code

@Smat26
Copy link
Contributor Author

Smat26 commented Mar 19, 2020

Thanks for reviewing.
I know hardcoding a package name + version is very inelegant but to answer your questions:

Why are you installing scipy in the update script - and hardcoding a specific version?

Yes there is a broken dependency.
If we execute the very next command (pip installing ../base_packages/requirements.txt), it fails giving a dependency error. Doing a pip install before it fixes it.

If there is a reason not to trust dependencies or installing dependencies with our extra pip options messes things up, base_package/requirements.txt is the place - and you can see python version specific logic.

So the logical step is to add this dependency in the ../base_packages/requirements.txt.
That is what I did at first, but the dependency error persisted.
This is due to the issue mentioned here:
pypa/pip#1386
and
https://superuser.com/questions/1390544/installing-python-deps-with-pip-r-requirements-txt-fails-but-installing-one-by

So now there are essentially two set of constraints:

  1. This scipy needs to be installed as a part of dependency.
  2. Because of the way pip is, we know dependency resolution will fail.

Version 1.2.3 is available for python 2. For python 3, we should move to the 1.4 series. Is there a reason for 1.2.x - there are ways to code that without coding a specific version - and making it depend on python 2 only.

This I am not sure of. My reasons for the version was the video, where scipy was installed for 1.2.2
However right now I checked, and it works for 1.2.3 as well, so the version doesn't need hardcoding.

I know putting a package name and installing isn't the best way. Do you have a good way we can do this given the constraints I highlighted above?
Maybe a two tier-ed installation with pre-requirements.txt and requirements.txt

@epeisach
Copy link
Contributor

I am thinking two different base ones might be required.... I will play with it over the weekend...

@Smat26
Copy link
Contributor Author

Smat26 commented Mar 19, 2020

scipy is pulled in by wwpdb-TEMPy for python2 which is part of the base install.

Yes

There is also an assumption of what directory you are sitting in.
open('../base_packages/requirements_wwpdb_dependencies.txt')
I do not see a chdir before this code

Oh yes.
Fixed.

@berrisfordjohn
Copy link
Contributor

can we remove wwpdb-TEMPy and biopython from the requirements.txt?
can these be installed as part of validation? or do they need to installed earlier?

Copy link
Contributor

@berrisfordjohn berrisfordjohn left a comment

Choose a reason for hiding this comment

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

this branch works for a blank install where as master doesn't.
@epeisach over to you to approve

@epeisach
Copy link
Contributor

epeisach commented Jun 8, 2020

Need to lock at 1.73 of biopython for python 2. Until va package is updated to not lock down version - you will probably wind up installing 1.76, then not being backed down.

With 1.76 installed - does "pip check" report any issues?

@epeisach
Copy link
Contributor

epeisach commented Jun 8, 2020

And if biopython did not put n proper metadata to tell pip this is not the version you want - shame on them. This is the fourth package with the issue. Numpy got it right, but other packages have not. We wind up having to hardcode versions in setup.py which is unfortunate.

@berrisfordjohn
Copy link
Contributor

can biopython be removed from requirements.txt? why does it need to be here?

@berrisfordjohn
Copy link
Contributor

@epeisach can this be merged to master now?

@berrisfordjohn
Copy link
Contributor

i've just used this to install 3 servers (production, staging and a development server with --edit). it worked for me. @epeisach can we merge please.

@berrisfordjohn
Copy link
Contributor

@epeisach, can you merge this? @sanjaab just ran into the problems that this solves when trying to set up her own machine

@epeisach epeisach merged commit 8476107 into master Jul 12, 2020
@epeisach
Copy link
Contributor

I have merged your pull request, Looks correct to me.
You can delete your branch it you would like.

@Smat26 Smat26 deleted the pvt-ahsant-dev-isntall-in-RunUpdate branch July 21, 2020 10:23
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.

None yet

3 participants