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

Split insight/prediction (part 2) #556

Merged
merged 27 commits into from
Jan 28, 2022
Merged

Conversation

mithridatea
Copy link
Contributor

@mithridatea mithridatea commented Jan 24, 2022

2nd part of insight refactoring (see #544).

Create a new Prediction table, store all predictions in this table. Keep the insight import logic as it is.

All latent insights will be moved to the Prediction table, it should greatly reduces the API latency issue described here: #567.

Latent insight migration script, to be run after the version is deployed:

WITH to_copy_rows AS (
    SELECT
        barcode,
        type,
        data,
        timestamp,
        value_tag,
        value,
        source_image,
        automatic_processing,
        server_domain,
        predictor
    FROM
        product_insight
    WHERE
        latent is true
)
INSERT INTO
    prediction(
        barcode,
        type,
        data,
        timestamp,
        value_tag,
        value,
        source_image,
        automatic_processing,
        server_domain,
        predictor
    )
SELECT
    *
FROM
    to_copy_rows;

DELETE FROM
    product_insight
WHERE
    latent IS TRUE;

@mithridatea mithridatea changed the title Split prediction insight 2 Slit insight/prediction (part 2) Jan 24, 2022
@mithridatea mithridatea marked this pull request as ready for review January 24, 2022 10:20
@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

Merging #556 (1e63ca2) into master (30dc472) will increase coverage by 7.05%.
The diff coverage is 60.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #556      +/-   ##
==========================================
+ Coverage   37.68%   44.73%   +7.05%     
==========================================
  Files         103       96       -7     
  Lines        7438     6981     -457     
==========================================
+ Hits         2803     3123     +320     
+ Misses       4635     3858     -777     
Impacted Files Coverage Δ
robotoff/cli/insights.py 0.00% <0.00%> (ø)
robotoff/health.py 0.00% <0.00%> (ø)
robotoff/metrics.py 25.80% <0.00%> (-1.32%) ⬇️
...prediction/category/prediction_from_ocr/cleaner.py 95.65% <ø> (ø)
...ediction/category/prediction_from_ocr/constants.py 100.00% <ø> (ø)
...ediction/category/prediction_from_ocr/predictor.py 95.83% <ø> (ø)
robotoff/prediction/langid.py 76.74% <ø> (ø)
robotoff/prediction/object_detection/__init__.py 100.00% <ø> (ø)
robotoff/prediction/object_detection/download.py 0.00% <ø> (ø)
...rediction/object_detection/utils/label_map_util.py 22.22% <ø> (ø)
... and 74 more

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 89c1c13...1e63ca2. Read the comment docs.

@mithridatea mithridatea changed the title Slit insight/prediction (part 2) Split insight/prediction (part 2) Jan 24, 2022
Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

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

This is mostly style comments, but a possible non negated condition.

I've seen you removed the init.py files in tests structure. I reread https://docs.pytest.org/en/6.2.x/goodpractices.html#choosing-a-test-layout-import-rules and I'm ok with that, but I will add a check that there are no filename duplicates in make tests !

robotoff/app/core.py Show resolved Hide resolved
robotoff/insights/importer.py Show resolved Hide resolved
robotoff/insights/importer.py Show resolved Hide resolved
robotoff/insights/importer.py Show resolved Hide resolved
robotoff/insights/importer.py Show resolved Hide resolved
robotoff/insights/importer.py Outdated Show resolved Hide resolved
robotoff/insights/importer.py Outdated Show resolved Hide resolved
robotoff/insights/importer.py Show resolved Hide resolved
mithridatea and others added 4 commits January 28, 2022 11:14
Co-authored-by: Alex Garel <alex@garel.org>
Co-authored-by: Alex Garel <alex@garel.org>
Retrieve existing prediction in batch (same barcode).
@sonarcloud
Copy link

sonarcloud bot commented Jan 28, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 28 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

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

Ok, let's go !

@mithridatea mithridatea merged commit a2b09e1 into master Jan 28, 2022
@mithridatea mithridatea deleted the split-prediction-insight-2 branch January 28, 2022 14:51
@alexgarel
Copy link
Member

I tested the DB modifications in preprod but adjusting to drop indexes before and recreate them after, because this is much more efficient.

Here is the script https://gist.github.com/alexgarel/c139f5dd8ed79506bde564a38b6a437a

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