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.

Sign up for GitHub

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

enforce order of installing python dependencies #1149

Merged
merged 20 commits into from
Dec 2, 2020
Merged

enforce order of installing python dependencies #1149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
97c2629
fix python install order
jrhizor Dec 1, 2020
8fec8ce
fix base install reqs dependencies
jrhizor Dec 1, 2020
9f8355f
don't clobber dependencies
jrhizor Dec 1, 2020
a23606d
remove cacheIf forcing
jrhizor Dec 1, 2020
8095db3
it wasn't clobbering actually
jrhizor Dec 1, 2020
a6c147d
fix pip version
jrhizor Dec 1, 2020
5a4eb3b
loosen pyyaml version
jrhizor Dec 1, 2020
2652600
Revert "loosen pyyaml version"
jrhizor Dec 1, 2020
da3ab27
move tap-adwords dependency
jrhizor Dec 1, 2020
256cf55
downgrade shopify api version used to match
jrhizor Dec 1, 2020
c06482f
run seed init
jrhizor Dec 1, 2020
caee9ee
add test curl
jrhizor Dec 1, 2020
84f58be
Revert "add test curl"
jrhizor Dec 1, 2020
64071fc
revert to simpler docker times
jrhizor Dec 1, 2020
81f1fc1
rename task function so it doesn't match the task name
jrhizor Dec 1, 2020
f08fd37
turn off buildkit
jrhizor Dec 1, 2020
f4bfe21
fix redshift dir for credentials
jrhizor Dec 2, 2020
dbc779d
add twilio and freshdesk to seed yaml
jrhizor Dec 2, 2020
afe7299
fix typo
jrhizor Dec 2, 2020
011a911
get shopify passing similar to the adwords fix
jrhizor Dec 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 0 additions & 14 deletions .github/workflows/gradle.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,6 @@ jobs:
- name: Check images exist
run: ./tools/bin/check_images_exist.sh

# # docker should use restore-keys since these are just used for layer caching, not gradle task caching
# - name: Docker Caching
# uses: actions/cache@v2
# with:
# path: |
# /tmp/docker-registry
# key: ${{ secrets.CACHE_VERSION }}-docker-${{ runner.os }}-${{ hashFiles('Dockerfile') }}-${{ github.sha }}
# restore-keys: |
# ${{ secrets.CACHE_VERSION }}-docker-${{ runner.os }}-${{ hashFiles('Dockerfile') }}-
# ${{ secrets.CACHE_VERSION }}-docker-${{ runner.os }}-

- name: Pip Caching
uses: actions/cache@v2
with:
Expand Down Expand Up @@ -66,9 +55,6 @@ jobs:
with:
python-version: '3.7'

- name: Start local Docker registry
run: docker run -d -p 5000:5000 --restart=always --name registry -v /tmp/docker-registry:/var/lib/registry registry:2 && npx wait-on tcp:5000

- name: Build
run: ./gradlew --no-daemon build --scan

Expand Down
2 changes: 1 addition & 1 deletion ...ain/resources/config/STANDARD_SOURCE_DEFINITION/fdc8b827-3257-4b33-83cc-106d234c34d4.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@
"sourceDefinitionId": "fdc8b827-3257-4b33-83cc-106d234c34d4",
"name": "Google Adwords",
"dockerRepository": "airbyte/source-google-adwords-singer",
"dockerImageTag": "0.1.3",
"dockerImageTag": "0.1.4",
"documentationUrl": "https://hub.docker.com/r/airbyte/source-google-adwords"
}
12 changes: 11 additions & 1 deletion airbyte-config/init/src/main/resources/seed/source_definitions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
- sourceDefinitionId: fdc8b827-3257-4b33-83cc-106d234c34d4
name: Google Adwords
dockerRepository: airbyte/source-google-adwords-singer
dockerImageTag: 0.1.3
dockerImageTag: 0.1.4
documentationUrl: https://hub.docker.com/r/airbyte/source-google-adwords
- sourceDefinitionId: ef69ef6e-aa7f-4af1-a01d-ef775033524e
name: Github
Expand Down Expand Up @@ -98,3 +98,13 @@
dockerRepository: airbyte/source-redshift
dockerImageTag: 0.1.0
documentationUrl: https://hub.docker.com/repository/docker/airbyte/source-redshift
- sourceDefinitionId: 932e6363-d006-4464-a9f5-102b82e07c06
name: Twilio
dockerRepository: airbyte/source-twilio-singer
dockerImageTag: 0.1.0
documentationUrl: https://hub.docker.com/r/airbyte/source-twilio-singer
- sourceDefinitionId: ec4b9503-13cb-48ab-a4ab-6ade4be46567
name: Freshdesk
dockerRepository: airbyte/source-freshdesk
dockerImageTag: 0.1.0
documentationUrl: https://hub.docker.com/r/airbyte/source-freshdesk
1 change: 1 addition & 0 deletions airbyte-integrations/bases/airbyte-protocol/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ task generateProtocolClassFilesWithoutLicense(type: Exec) {
task generateProtocolClassFiles(dependsOn: [generateProtocolClassFilesWithoutLicense, rootProject.spotlessPython])
rootProject.spotlessPython.mustRunAfter(generateProtocolClassFilesWithoutLicense)
rootProject.build.dependsOn generateProtocolClassFiles
installReqs.dependsOn(generateProtocolClassFiles)
2 changes: 2 additions & 0 deletions airbyte-integrations/bases/base-normalization/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,5 @@ airbytePython {
dependencies {
implementation files(project(':airbyte-integrations:bases:airbyte-protocol').airbyteDocker.outputs)
}

installReqs.dependsOn(":airbyte-integrations:bases:airbyte-protocol:installReqs")
Copy link
Contributor

Choose a reason for hiding this comment

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

how did you think about picking :installReqs versus just depending on the build of that module? i'm wondering if we are going to make a bad assumption in the future. i'm also fine with keeping this though if you think it's the right way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should go away in the future, and it may go away in favor of the build step. However, the build step in the future might not match what we have now, so I think this is the lightest weight shim to tide us over.

2 changes: 2 additions & 0 deletions airbyte-integrations/bases/base-python-test/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,5 @@ airbytePython {
dependencies {
implementation files(project(':airbyte-integrations:bases:base-python').airbyteDocker.outputs)
}

installReqs.dependsOn(":airbyte-integrations:bases:airbyte-protocol:installReqs")
2 changes: 2 additions & 0 deletions airbyte-integrations/bases/base-python/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,5 @@ dependencies {
implementation files(project(':airbyte-integrations:bases:airbyte-protocol').airbyteDocker.outputs)
implementation files(project(':airbyte-integrations:bases:base').airbyteDocker.outputs)
}

installReqs.dependsOn(":airbyte-integrations:bases:airbyte-protocol:installReqs")
2 changes: 2 additions & 0 deletions airbyte-integrations/bases/base-singer/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,5 @@ airbytePython {
dependencies {
implementation files(project(':airbyte-integrations:bases:base-python').airbyteDocker.outputs)
}

installReqs.dependsOn(":airbyte-integrations:bases:airbyte-protocol:installReqs", ":airbyte-integrations:bases:base-python:installReqs")
3 changes: 2 additions & 1 deletion airbyte-integrations/connectors/source-google-adwords-singer/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ ENV AIRBYTE_IMPL_PATH="SourceGoogleAdwordsSinger"
WORKDIR /airbyte/integration_code
COPY $CODE_PATH ./$CODE_PATH
COPY setup.py ./
RUN pip install tap-adwords==1.12.0
RUN pip install ".[main]"

LABEL io.airbyte.version=0.1.3
LABEL io.airbyte.version=0.1.4
LABEL io.airbyte.name=airbyte/source-google-adwords-singer
7 changes: 7 additions & 0 deletions airbyte-integrations/connectors/source-google-adwords-singer/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,10 @@ airbytePython {
dependencies {
implementation files(project(':airbyte-integrations:bases:base-singer').airbyteDocker.outputs)
}

// used to allow local iteration to work
Copy link
Contributor

Choose a reason for hiding this comment

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

so should we be doing this in all singer sources or just this one if just this one can you explain why in the comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

if all of them do we need to document it? put it in the template, etc. okay if this happens in a separae commit if that's the case.

Copy link
Contributor Author

@jrhizor jrhizor Dec 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally we do all of them, but it's lower priority than getting something working. Since we only had two cases where it didn't align, and both are easily fixable with this, it seems easiest to do this here. If we split it for every singer source, we shouldn't do it here; it should be part of a singer integration plugin.

task('installSingerTap', type: PythonTask) {
module = "pip"
command = "install tap-adwords==1.12.0"
}
installReqs.dependsOn installSingerTap
2 changes: 1 addition & 1 deletion airbyte-integrations/connectors/source-google-adwords-singer/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
# Dependencies required by the main package but not integration tests should go in main. Deps required by
# integration tests but not the main package go in integration_tests. Deps required by both should go in
# install_requires.
"main": ["base-singer", "base-python", "tap-adwords==1.12.0"],
"main": ["base-singer", "base-python"],
"tests": ["airbyte-python-test", "pytest"],
},
)
2 changes: 2 additions & 0 deletions airbyte-integrations/connectors/source-shopify-singer/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ ENV AIRBYTE_IMPL_PATH="SourceShopifySinger"
WORKDIR /airbyte/integration_code
COPY $CODE_PATH ./$CODE_PATH
COPY setup.py ./
RUN pip install pip==20.3
RUN pip install tap-shopify==1.2.6
RUN pip install ".[main]"

LABEL io.airbyte.version=0.1.4
Expand Down
2 changes: 2 additions & 0 deletions airbyte-integrations/connectors/source-shopify-singer/Dockerfile.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ COPY secrets/config.json $CODE_PATH/config.json
COPY $MODULE_NAME/*.json $CODE_PATH/
COPY setup.py ./

RUN pip install pip==20.3
RUN pip install tap-shopify==1.2.6
RUN pip install ".[tests]"

WORKDIR /airbyte
7 changes: 7 additions & 0 deletions airbyte-integrations/connectors/source-shopify-singer/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,10 @@ airbytePython {
dependencies {
implementation files(project(':airbyte-integrations:bases:base-singer').airbyteDocker.outputs)
}

// used to allow local iteration to work
task('installSingerTap', type: PythonTask) {
module = "pip"
command = "install tap-shopify==1.2.6"
}
installReqs.dependsOn installSingerTap
2 changes: 1 addition & 1 deletion airbyte-integrations/connectors/source-shopify-singer/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
author="Airbyte",
author_email="[email protected]",
packages=find_packages(),
install_requires=["airbyte-protocol", "tap-shopify==1.2.6", "ShopifyAPI==8.0.4"],
install_requires=["airbyte-protocol", "ShopifyAPI==8.0.4"],
package_data={"": ["*.json"]},
setup_requires=["pytest-runner"],
tests_require=["pytest"],
Expand Down
2 changes: 1 addition & 1 deletion buildSrc/src/main/groovy/airbyte-docker.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ abstract class AirbyteDockerTask extends DefaultTask {
}

@TaskAction
def airbyteDocker() {
def dockerTask() {
def scriptPath = Paths.get(rootDir.absolutePath, 'tools/bin/build_image.sh').toString()
buildDockerfile(scriptPath, dockerfileName)
}
Expand Down
9 changes: 6 additions & 3 deletions buildSrc/src/main/groovy/airbyte-python.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class AirbytePythonPlugin implements Plugin<Project> {
pip 'mypy:0.790'
pip 'isort:5.6.4'
pip 'pytest:6.1.2'
pip 'pip:20.3'
}

project.task('blackFormat', type: PythonTask) {
Expand All @@ -48,23 +49,25 @@ class AirbytePythonPlugin implements Plugin<Project> {
command = "install -r requirements.txt"
inputs.file('requirements.txt')
outputs.file('build/installedlocalreqs.txt')
outputs.cacheIf { true }

// HACK: makes all integrations depend on installing requirements for bases. long term we should resolve deps and install in order.
Copy link
Contributor

Choose a reason for hiding this comment

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

long term we should resolve deps and install in order.

what will that look like? should we just have an airbyte-integration.gradle that handles this stuff? again. doesn't have to happen now, just want to understand how you're thinking about it and if possible create an issue with those thoughts.

if you add issue also add its number to the comment.

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'm still figuring out precisely how to do this. I'll post on here later with the improvement ticket, but this HACK should be gone by EOW.

if(project.getPath().startsWith(":airbyte-integrations:connectors")) {
dependsOn project.rootProject.getTasksByName("airbytePythonApply", true).findAll { it.project.getPath().startsWith(":airbyte-integrations:bases")}
}
}

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

project.task('installTestReqs', type: PythonTask, dependsOn: project.installReqs) {
module = "pip"
command = "install .[tests]"
inputs.file('setup.py')
outputs.file('build/installedtestreqs.txt')
outputs.cacheIf { true }
}

if(project.file('unit_tests').exists()) {
Expand Down
22 changes: 4 additions & 18 deletions tools/bin/build_image.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,7 @@ assert_root

cd "$PROJECT_DIR"

if [[ -z "$CI" ]]; then
# run standard build locally (not on CI)
DOCKER_BUILDKIT=1 docker build \
-f "$DOCKERFILE" . \
-t "$TAGGED_IMAGE" \
--iidfile "$ID_FILE"
else
# run build with local docker registery for CI
docker pull localhost:5000/"$TAGGED_IMAGE" || true
DOCKER_BUILDKIT=1 docker build \
-f "$DOCKERFILE" . \
-t "$TAGGED_IMAGE" \
--iidfile "$ID_FILE" \
--cache-from localhost:5000/"$TAGGED_IMAGE" \
--build-arg BUILDKIT_INLINE_CACHE=1
docker tag "$TAGGED_IMAGE" localhost:5000/"$TAGGED_IMAGE"
docker push localhost:5000/"$TAGGED_IMAGE"
fi
docker build \
-f "$DOCKERFILE" . \
-t "$TAGGED_IMAGE" \
--iidfile "$ID_FILE"
2 changes: 1 addition & 1 deletion tools/bin/ci_credentials.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ echo "$BIGQUERY_INTEGRATION_TEST_CREDS" > "${SOURCEFILE_DIR}/gcs.json"
echo "$AWS_S3_INTEGRATION_TEST_CREDS" > "${SOURCEFILE_DIR}/aws.json"

REDSHIFT_DIR=airbyte-integrations/connectors/source-redshift/secrets
mkdir REDSHIFT_DIR
mkdir $REDSHIFT_DIR
echo "$AWS_REDSHIFT_INTEGRATION_TEST_CREDS" > "${REDSHIFT_DIR}/config.json"

MAILCHIMP_SECRETS_DIR=airbyte-integrations/connectors/source-mailchimp/secrets
Expand Down