[Outreachy Round 27] Refactor `LiftWingApi` and `RevisionScoreImporter` #5578

gabina · 2024-01-09T18:45:37Z

What this PR does

This PR is part of the "Improve how Wiki Education Dashboard counts references added" project (read issue #5547).

It makes some refactors on LiftWingApi and RevisionScoreImporter classes.

The RevisionScoreImporter is responsible for populating fields such as wp10, features, wp10_previous, features_previous, and deleted in the revisions. Currently, this involves using the LiftWing API to retrieve data. However, as part of the "Improve how Wiki Education Dashboard counts references added" project, we also need to use the new reference-counter API (see #5565).

The primary idea is to maintain the RevisionScoreImporter as the class responsible for populating all revision score fields, regardless of whether the values come from the LiftWing API or the Toolforge reference-counter API. This approach offers benefits, as there is valuable logic that should be shared in the population process, irrespective of the data source. For example, the get_parent_revisions method retrieves parent revision IDs, necessary for updating both wp10_previous (from the LiftWing API) and features_previous (from the Toolforge reference-counter API).

Before this PR, the importer contained a lot of logic specific to the articlequality model data and how to process it (such as calculating the wp10 value from the scores).

The purpose of this refactor is to move elements too specific to the LiftWing API into the existing LiftWingApi class. This way, RevisionScoreImporter becomes a class with the logic to populate fields from both LiftWingApi and ReferenceCounterApi.

Open questions and concerns

< anything you learned that you want to share, or questions you're wondering about related to this PR >

…elated to it

spec/services/update_course_stats_spec.rb

…e is an error hitting the API

gabina · 2024-01-11T20:50:27Z

lib/importers/revision_score_importer.rb

+      revision.wp10 = score.dig('wp10')
+      revision.features = score.dig('features')
+      # only modify the existing deleted value if revision was deleted
+      revision.deleted = true if score.dig('deleted')


The spec modified in c045872 was to be sure that revision.deleted is not updated if score.dig('deleted') isn't set to true. If score.dig('deleted') is nil (because score is the empty hash {}), then revision.deleted is not updated.

gabina · 2024-01-11T20:53:07Z

lib/lift_wing_api.rb

+  # Given an array of revision ids, it returns an array with useful metrics for those
+  # revision ids.
+  # Format result example:
+  # { 'rev_id0' => { 'wp10' => 0.2915228958136511656e2, 'features' => features_value,
+  #                 'deleted' => false, 'prediction' => 'Stub' }
+  #   ...
+  #   'rev_idn' => { 'wp10' => 0.285936675221734978e2, 'features' => features_value,
+  #                 'deleted' => false, 'prediction' => 'D' }
+  # }


I added this as I don't think there is any other way to specify types in ruby
There is a mistake though: Given an array of revision ids, it returns a hash with useful metrics for those

gabina · 2024-01-11T20:56:51Z

lib/lift_wing_api.rb

 # https://wikitech.wikimedia.org/wiki/Machine_Learning/LiftWing
-class LiftWingApi
+class LiftWingApi # rubocop:disable Metrics/ClassLength


I had to disable the rule because the class is too long.
One option could be to move the WEIGHTING constants outside the class? Other thing that would take more effort would be to split the LfitWingApi class in two different classes: one to get data form the api, and other one to process it (calculating the wp10 for example).

The better place to do this is .rubocop.yml so that we have a central list of all the too-big classes.

gabina · 2024-01-11T20:57:24Z

lib/lift_wing_api.rb

-  def get_single_revision_data(rev_id)
+  private
+
+  # Returns a has with wp10, features, deleted, and prediction, or empty hash if


typo here: Returns a hash

gabina · 2024-01-11T20:57:53Z

lib/lift_wing_api.rb

    body = { rev_id:, extended_output: true }.to_json
    response = lift_wing_server.post(quality_query_url, body)
    parsed_response = Oj.load(response.body)

-    return equivalent_ores_error_response(rev_id, parsed_response) if parsed_response.key? 'error'
+    # If the responses contains an error, do not try to calculate wp10 or features.


typo here: If the responses contain

gabina · 2024-01-11T21:00:00Z

spec/lib/importers/revision_score_importer_spec.rb

@@ -57,22 +57,6 @@
    end
  end

-  it 'saves wp10 scores by article' do
-    VCR.use_cassette 'revision_scores/by_article' do
-      described_class.update_revision_scores_for_all_wikis


Removed all the specs calling update_revision_scores_for_all_wikis because removed the method.

gabina · 2024-01-12T18:12:15Z

lib/lift_wing_api.rb

-        'error' => { 'message' => error_response['error'], 'type' => type }
-        } } } } }
-  end
-
  # TODO: monitor production for errors, understand them, put benign ones here
  TYPICAL_ERRORS = [].freeze

  def log_error_batch(rev_ids)
    return if @errors.empty?

    log_error(@errors.first, update_service: @update_service,


I understand that we always log the first error, even when there might be multiple distinct errors. I guess we work under the assumption that if several requests fail, it's likely for the same reason, which may hold true in most cases. However, I suggest resetting the @errors array in get_revision_data. Without this adjustment, if there are two batches of requests, each with a single request failing, we would log the first error twice and miss the second one entirely.

Done on cde5e1d

ragesoss · 2024-01-16T20:23:53Z

lib/lift_wing_api.rb

+    @weighting ||= WEIGHTING_BY_LANGUAGE[@wiki.language]
+  end
+
+  def weighted_mean_score(score)


I think this whole set of weighting code might make more sense as a separate file, since turning articlequality predictions into a single score is not a concept inherent to the LiftWing API.

Perhaps it can be extracted to a module that LiftWingApi includes (similar to ConstantUpdate and its included module BatchUpdateLogging).

I like the idea! Added the new module on d0dfb96, and modified the LiftWingApi class to include the module on 18c7a40.
Didn't add specific specs for the new module because it's only used in the LiftWingApi, and the LiftWingApi
specs already check that the weighted mean score makes sense.

ragesoss · 2024-01-16T20:26:16Z

Overall this looks solid. I left a couple of suggestions; other than that, it looks ready to me. @Aminehassou would you like to have a look as well?

Aminehassou

Outside of the things Sage already mentioned, LGTM!

…ilities into a single score

gabina · 2024-01-17T15:46:19Z

spec/lib/lift_wing_api_spec.rb

@@ -70,15 +75,24 @@
    it 'handles timeout errors' do
      stub_request(:any, 'https://api.wikimedia.org')
        .to_raise(Errno::ETIMEDOUT)
-      expect_any_instance_of(described_class).to receive(:log_error).once
+      expect(lift_wing_api_class_en_wiki).to receive(:log_error).once


Changed this to avoid using expect_any_instance_of here, as the spec then makes statements on subject0, which is a particular instance. I don't like making expectations to any instance and to a particular instance in the same spec, I think it's confusing.

Refactor liftwing api class and revision score importer.

0a06d26

gabina marked this pull request as draft January 9, 2024 18:46

gabina added 3 commits January 10, 2024 12:01

Update Lift Wing description

e311876

Fix UpdateCourseStats spec

b344ac1

Remove unused update_revision_scores_for_all_wikis method and specs r…

ba25f80

…elated to it

gabina commented Jan 10, 2024

View reviewed changes

spec/services/update_course_stats_spec.rb Outdated Show resolved Hide resolved

gabina added 7 commits January 10, 2024 20:41

Roll back some changes as we want to log errors in Sentry at batch level

c490de0

Fix lint

c6936a4

Remove unused method

99874ff

Add comments and improve specs

89b9b03

Add comments

f8c0b01

Replace ores with liftwing in variable and method names

0fe468b

Add one spec to be sure that any revision value is modified when ther…

c045872

…e is an error hitting the API

gabina commented Jan 11, 2024

View reviewed changes

gabina marked this pull request as ready for review January 11, 2024 21:00

gabina changed the title ~~[WIP] [Outreachy Round 27] Refactor LiftWingApi and RevisionScoreImporter~~ [Outreachy Round 27] Refactor LiftWingApi and RevisionScoreImporter Jan 11, 2024

gabina requested a review from ragesoss January 11, 2024 21:01

gabina commented Jan 12, 2024

View reviewed changes

Restart the errors array when getting data from a new batch of revisions

cde5e1d

gabina mentioned this pull request Jan 16, 2024

Improve how Wiki Education Dashboard counts references added #5547

Open

10 tasks

ragesoss reviewed Jan 16, 2024

View reviewed changes

Aminehassou approved these changes Jan 17, 2024

View reviewed changes

gabina added 2 commits January 17, 2024 12:18

Add a module with the logic to turns articlequality prediction probab…

d0dfb96

…ilities into a single score

Use the WeightedScoreCalculator module in the LiftWingApi class

18c7a40

gabina commented Jan 17, 2024

View reviewed changes

gabina requested a review from ragesoss January 17, 2024 16:40

ragesoss merged commit b99cb4f into WikiEducationFoundation:master Jan 17, 2024
1 check passed

gabina deleted the 5547-outreachy-round-27-refactor-revision-score-importer-and-liftwing-api branch January 17, 2024 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Outreachy Round 27] Refactor `LiftWingApi` and `RevisionScoreImporter` #5578

[Outreachy Round 27] Refactor `LiftWingApi` and `RevisionScoreImporter` #5578

gabina commented Jan 9, 2024

gabina Jan 11, 2024

gabina Jan 11, 2024 •

edited

Loading

gabina Jan 11, 2024

ragesoss Jan 16, 2024

gabina Jan 11, 2024

gabina Jan 11, 2024

gabina Jan 11, 2024

gabina Jan 12, 2024

gabina Jan 12, 2024

ragesoss Jan 16, 2024

gabina Jan 17, 2024

ragesoss commented Jan 16, 2024

Aminehassou left a comment

gabina Jan 17, 2024

[Outreachy Round 27] Refactor LiftWingApi and RevisionScoreImporter #5578

[Outreachy Round 27] Refactor LiftWingApi and RevisionScoreImporter #5578

Conversation

gabina commented Jan 9, 2024

What this PR does

Open questions and concerns

Choose a reason for hiding this comment

gabina Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ragesoss commented Jan 16, 2024

Aminehassou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Outreachy Round 27] Refactor `LiftWingApi` and `RevisionScoreImporter` #5578

[Outreachy Round 27] Refactor `LiftWingApi` and `RevisionScoreImporter` #5578

gabina Jan 11, 2024 •

edited

Loading