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

Install python deps declared in setup.py #1111

Merged
merged 3 commits into from
Nov 30, 2020
Merged

Install python deps declared in setup.py #1111

merged 3 commits into from
Nov 30, 2020

Conversation

cgardens
Copy link
Contributor

@cgardens cgardens commented Nov 28, 2020

Relates to #1110
Relates to #1031

What

  • I have not been able to run ./gradlew clean build successfully on my local machine for the last 2 weeks due to the error described here: ./gradlew clean build fails  #1031. It seems to be a race condition-y sort of thing that happens when building the whole project.
  • It seems like we don't ever install deps declared in setup.py. We just install what's in requirements.txt. Not 100% sure if this is a separate problem from the previous bullet point or related. The fix for this seems to have fixed the previous bullet point as well, but I'm not totally clear on why.
  • I think this got confused because of our need to install local python deps that are located in the project

How

  • Keeps step to install our own local deps in each project that depends on them
  • Add step that installs deps in setup.py
  • Not sure this is the right way to do this... I was trying to figure out how to move the local dep installation into setup.py but wasn't successful.

Next Steps

  • It seems like the way this should work is that each library produces a self-contained artifact. I think this should be doable as python eggs. Then each module that depends on the library can just consume the egg and we don't need to rely on requirements.txt at all.

@@ -1,3 +1,2 @@
-e ../airbyte-protocol
-e .
dbt==0.18.1
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 think someone put a non-local dependency here, because setup.py wasn't doing the installation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I added that there at some point to make some task work somehow... Not sure if you remove it, it's going to do well or not...

The way Airbyte is building python packages is really weird and i don't think it's following proper python recommendations, for example I don't understand why we do some things in setup.py and are using requirements.txt files in this way... (see here

Then lately i also run in such build failures that could be related:
Screenshot 2020-11-30 at 14 21 26

project.task('installReqs', type: PythonTask, dependsOn: project.installLocalReqs) {
module = "pip"
command = "install .[main]"
inputs.file('setup.py')
outputs.file('build/installedreqs.txt')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do these files exist? i haven't been able to find them. pattern matched the naming convention above.

Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

This fixes the issue but I think we should consider dropping the [main] convention since it's supposed to be used for extra/truly optional deps whereas everything that goes in main is required for the module to function.

@cgardens cgardens merged commit 31c3f06 into master Nov 30, 2020
@cgardens cgardens deleted the cgardens/python branch November 30, 2020 08:57
@ChristopheDuong
Copy link
Contributor

Next Steps:
It seems like the way this should work is that each library produces a self-contained artifact. I think this should be doable as python eggs. Then each module that depends on the library can just consume the egg and we don't need to rely on requirements.txt at all.

FYI with pants build tool with python, it is geared towards using pex to do this: https://pex.readthedocs.io/en/latest/

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