Skip to content

Use --no-deps when installing compiled requirements files #2752

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

Merged
merged 5 commits into from
Feb 24, 2022
Merged

Conversation

relud
Copy link
Collaborator

@relud relud commented Feb 24, 2022

to work around pypa/pip#9644

see also mozilla/gcp-ingestion#2001

Checklist for reviewer:

  • Commits should reference a bug or github issue, if relevant (if a bug is referenced, the pull request should include the bug number in the title)
  • If the PR comes from a fork, trigger integration CI tests by running the Push to upstream workflow and provide the <username>:<branch> of the fork as parameter. The parameter will also show up
    in the logs of the manual-trigger-required-for-fork CI task together with more detailed instructions.
  • If adding a new field to a query, ensure that the schema and dependent downstream schemas have been updated

For modifications to schemas in restricted namespaces (see CODEOWNERS):

@relud relud requested a review from sean-rose February 24, 2022 18:38
Comment on lines -8 to -11
google-api-core==1.31.5 # transitive dep that needs dependabot updates
google-cloud-bigquery==2.34.0
google-cloud-storage==2.1.0
googleapis-common-protos==1.54.0 # transitive dep that needs dependabot updates
Copy link
Collaborator Author

@relud relud Feb 24, 2022

Choose a reason for hiding this comment

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

remove previous workaround from #2064 which was having no impact

@dataops-ci-bot
Copy link

Integration report for "Use --no-deps when installing compiled requirements files"

sql.diff

Click to expand!
diff -bur --no-dereference bigquery-etl-main/sql/moz-fx-data-shared-prod/telemetry/clients_daily/schema.yaml /tmp/workspace/generated-sql/sql/moz-fx-data-shared-prod/telemetry/clients_daily/schema.yaml
--- bigquery-etl-main/sql/moz-fx-data-shared-prod/telemetry/clients_daily/schema.yaml	2022-02-24 18:52:00.418582582 +0000
+++ /tmp/workspace/generated-sql/sql/moz-fx-data-shared-prod/telemetry/clients_daily/schema.yaml	2022-02-24 18:45:22.000000000 +0000
@@ -2,97 +2,74 @@
 - mode: NULLABLE
   name: submission_date_s3
   type: DATE
-- description: null
-  mode: NULLABLE
+- mode: NULLABLE
   name: submission_date
   type: DATE
 - mode: NULLABLE
   name: sample_id
   type: INTEGER
-- description: null
-  mode: NULLABLE
+- mode: NULLABLE
   name: client_id
   type: STRING
-- description: null
-  mode: NULLABLE
+- mode: NULLABLE
   name: aborts_content_sum
   type: INTEGER
-- description: null
-  mode: NULLABLE
+- mode: NULLABLE
   name: aborts_gmplugin_sum
   type: INTEGER
-- description: null
-  mode: NULLABLE
+- mode: NULLABLE
   name: aborts_plugin_sum
   type: INTEGER
-- description: null
-  mode: NULLABLE
+- mode: NULLABLE
   name: active_addons_count_mean
   type: FLOAT
-- description: null
-  fields:
-  - description: null
-    mode: NULLABLE
+- fields:
+  - mode: NULLABLE
     name: addon_id
     type: STRING
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: blocklisted
     type: BOOLEAN
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: name
     type: STRING
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: user_disabled
     type: BOOLEAN
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: app_disabled
     type: BOOLEAN
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: version
     type: STRING
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: scope
     type: INTEGER
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: type
     type: STRING
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: foreign_install
     type: BOOLEAN
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: has_binary_components
     type: BOOLEAN
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: install_day
     type: INTEGER
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: update_day
     type: INTEGER
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: signed_state
     type: INTEGER
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: is_system
     type: BOOLEAN
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: is_web_extension
     type: BOOLEAN
-  - description: null
-    mode: NULLABLE
+  - mode: NULLABLE
     name: multiprocess_compatible
     type: BOOLEAN
   mode: REPEATED

Copy link
Contributor

@sean-rose sean-rose left a comment

Choose a reason for hiding this comment

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

You read my mind! (thanks for tackling this)

  • There are some pip install commands in .circleci/config.yml which also need to be updated.
  • A similar change needs to be made wherever pip-sync is used (e.g. --pip-args "--no-deps"), or pip-sync could be swapped out for pip install.
    • On a related note, in the readme's "Installing bqetl" section, running pip-sync in step 4 ends up uninstalling bqetl from the venv because bqetl isn't in the requirements files, which would be a reason to use pip install instead.

@relud
Copy link
Collaborator Author

relud commented Feb 24, 2022

You read my mind! (thanks for tackling this)

* There are some `pip install` commands in `.circleci/config.yml` which also need to be updated.

👍

* A similar change needs to be made wherever pip-sync is used (e.g. `--pip-args "--no-deps"`), or pip-sync could be swapped out for `pip install`.

CI caches the pip venv, and needs to use pip-sync to uninstall removed dependencies to ensure reproducibility.

  * On a related note, in the readme's "Installing bqetl" section, running pip-sync in step 4 ends up uninstalling bqetl from the venv because bqetl isn't in the requirements files, which would be a reason to use `pip install` instead.

not using pip-sync is outside the scope of this PR

@relud relud requested a review from sean-rose February 24, 2022 19:54
@relud
Copy link
Collaborator Author

relud commented Feb 24, 2022

  * On a related note, in the readme's "Installing bqetl" section, running pip-sync in step 4 ends up uninstalling bqetl from the venv because bqetl isn't in the requirements files, which would be a reason to use `pip install` instead.

not using pip-sync is outside the scope of this PR

but we can retain bqetl with <(echo mozilla-bigquery-etl), which i've added.

@dataops-ci-bot
Copy link

Integration report for "don't erase bqetl with pip-sync"

sql.diff

Click to expand!
Only in bigquery-etl-main/sql/moz-fx-data-shared-prod/telemetry/experiment_cumulative_search_count: schema.yaml
Only in /tmp/workspace/generated-sql/sql/moz-fx-data-shared-prod/telemetry/main: schema.yaml

@@ -34,7 +34,7 @@ jobs:
command: |
python3.8 -m venv venv/
venv/bin/pip install pip-tools --constraint requirements.in
venv/bin/pip-sync
venv/bin/pip-sync --pip-args=--no-deps
Copy link
Contributor

Choose a reason for hiding this comment

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

By default I think pip-sync only looks for requirements.txt, so should this specify both requirements.txt and java-requirements.txt like is done elsewhere?

Suggested change
venv/bin/pip-sync --pip-args=--no-deps
venv/bin/pip-sync --pip-args=--no-deps requirements.txt \
java-requirements.txt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

java requirements are intentionally left out here, as they are optional and should not be required for unit tests.

@relud relud enabled auto-merge (squash) February 24, 2022 21:25
@dataops-ci-bot
Copy link

Integration report for "Merge branch 'main' into pip-no-deps"

sql.diff

No content detected.

@relud relud merged commit 4406437 into main Feb 24, 2022
@relud relud deleted the pip-no-deps branch February 24, 2022 21:36
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