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

Gpl 771 test fixes #216

Merged
merged 76 commits into from
Jan 19, 2021
Merged

Gpl 771 test fixes #216

merged 76 commits into from
Jan 19, 2021

Conversation

alexperrin1
Copy link
Contributor

Closes #167

Changes proposed in this pull request:

  • Batch legacy migration by dates (1 week took approx 15 mins)
  • Batch updates to Mongo in terms of total samples. Updates were already batched in filtered positive False/true
  • Batch MLWH updates by filtered positives true/false

* develop: (43 commits)
  Correct method documentation
  Do not incorrectly specify "positive" samples in the log message
  Raise an exception during the filtered positive rule change migration if the cherrypicked samples dataframe is None
  Update comments
  Renamed view for new view_plate_map
  Added --add-to-dart to Dockerfile
  Appease black and flake8
  Update tests
  Only add Result=Positive samples to DART. This will only affect the add to DART migration: the filtered positive rule change migration only passes RESULT=Positive samples to this method anyway
  Update add to DART helper method to fetch all samples between the timestamp range, not just Result=Positive ones
  Add events_warehouse database
  Add port to MLWH connection
  update in ci_docker as well as ci_python
  remove mysql root password from ci builds to match most people's local dev setups
  get rid of import - no longer used
  rename crawler service in docker compose to be consistent with other stack naming conventions
  don't keep connection config in the constants file, keep it in the default config file
  PR comment
  make connection string use the same constants as the username & password route - tests now pass locally
  Add comments detailing the scenario of each test
  ...

# Conflicts:
#	migrations/helpers/update_legacy_filtered_positives_helper.py
#	tests/conftest.py
#	tests/migrations/helpers/test_update_legacy_filtered_positives_helper.py
Copy link
Contributor

@Chris-Friend Chris-Friend left a comment

Choose a reason for hiding this comment

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

Looks good. No major comments

crawler/db.py Outdated Show resolved Hide resolved
crawler/db.py Show resolved Hide resolved
crawler/sql_queries.py Outdated Show resolved Hide resolved
@@ -199,3 +232,76 @@ def split_mongo_samples_by_version(
}

return samples_by_version


def update_mlwh_filtered_positive_fields_batched(
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned, I think the existing filtered positive rule change migration should call this more performant MLWH update method. Do you reckon it worth doing this as part of these changes, or a new "refactor" task, e.g. #173

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I think @KatyTaylor was creating a new task with filtered positive rule change migration changes, so I think that can be done as part of that task. I'm not sure if that task has been created yet though

Copy link
Contributor

Choose a reason for hiding this comment

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

@KatyTaylor, is this still to be done?

tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 15, 2021

Codecov Report

Merging #216 (5b0b43e) into develop (51b53b1) will increase coverage by 0.22%.
The diff coverage is 96.20%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #216      +/-   ##
===========================================
+ Coverage    91.23%   91.46%   +0.22%     
===========================================
  Files           26       26              
  Lines         1963     2086     +123     
  Branches       236      252      +16     
===========================================
+ Hits          1791     1908     +117     
- Misses         126      127       +1     
- Partials        46       51       +5     
Impacted Files Coverage Δ
...helpers/update_legacy_filtered_positives_helper.py 93.45% <90.56%> (-3.27%) ⬇️
crawler/db.py 97.04% <96.42%> (-0.15%) ⬇️
crawler/config/defaults.py 100.00% <100.00%> (ø)
crawler/constants.py 100.00% <100.00%> (ø)
crawler/sql_queries.py 100.00% <100.00%> (ø)
...ations/helpers/update_filtered_positives_helper.py 98.29% <100.00%> (+0.17%) ⬆️
migrations/update_legacy_filtered_positives.py 97.11% <100.00%> (+1.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51b53b1...5b0b43e. Read the comment docs.

@Chris-Friend
Copy link
Contributor

@KatyTaylor @pjvv I've made the changes Alex wasn't able to finish. Would you mind looking over the PR to check everything is in order?

@pjvv pjvv merged commit 0d104b5 into develop Jan 19, 2021
@pjvv pjvv deleted the GPL-771-test-fixes branch January 19, 2021 11:22
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.

GPL-771 Migrate all samples to use the new filtered_positive fields
3 participants