Sync test lists #50

hellais · 2018-10-09T15:46:07Z

This makes some changes to the sync-test-lists script which populates the relevant tables in the orchestra database for enabling the /api/v1/urls endpoint to work.

The workflow of this script is the following:

It checks if there is a history inside of the sync_test_lists table, if not it is considered to be a first run in which case it will populate the tables for the category codes, countries and writes all the URLs in the country lists.
It will pull (or clone if it's the first run) the citizenlab/test-list repository into the workingdir/test-lists
If the sync_test_lists contains a history, it will take the last commit hash and do a diff of that
Based on the diff it will look at which paths have changed and figure out if there are new URLs to add or if some were deleted and mark them as inactive (active = false)

Write the last synched commit to the sync_test_lists table

darkk · 2018-10-10T19:24:36Z

scripts/sync-test-lists.py

-);
-"""
-
-# XXX can a url belong to more than one category? (probably not)


BTW, it can (unless we've cleaned up that sort of weeds from test-lists).

This is not possible according to the citizenlab/test-lists format. This comment was mostly there if we maybe wanted to have some extra set of categories (multiple) for our own use, but I think that for the time being this is not that important.

That's to some extent possible with the citizenlab/test-lists format. One just adds same URL twice with different category codes:

ae.csv:http://www.haya.org,HUMR,Human Rights Issues,2014-04-15,citizenlab, bh.csv:http://www.haya.org,REL,Religion,2014-04-15,citizenlab,"Religious conversion, commentary and criticism" global.csv:https://www.facebook.com,GRP,Social Networking,2014-04-15,citizenlab,Updated by OONI on 2017-02-14 pk.csv:https://www.facebook.com,NEWS,News Media,2014-04-15,citizenlab, id.csv:http://denypagetests.netsweeper.com,NEWS,News Media,2014-04-15,citizenlab, kw.csv:http://denypagetests.netsweeper.com,CTRL,Control content,2014-04-15,citizenlab, global.csv:http://www.crazyshit.com,PORN,Pornography,2014-04-15,citizenlab,Updated by OONI on 2017-02-14 sg.csv:http://www.crazyshit.com,NEWS,News Media,2014-04-15,citizenlab,

I'm unsure if test-lists should be updated or data model should be extended...

That is, however, according to the data model of the DB, two distinct entries. The same URL present in two country specific lists is a different URL.

In general I think we should be doing better consistency checks in the test-lists URL to avoid dupe categories, but I don't see a problem in the DB datamodel.

It leads to several interesting questions:

when the orchestra gives URLs out and same URLs have different categories, should it give same URL twice? Probably, not.

if the user in SG does not want to test PORN but wants to test NEWS, should the user test crazyshit.com or not?

Filed: citizenlab/test-lists#400

There is third possible inconsistency: same url with trailing / for bare domain and without it.

bh.csv:http://arabi21.com/,NEWS,News Media,2017-11-13,citizenlab, eg.csv:http://arabi21.com,NEWS,News Media,2017-05-24,ADEF,blocked as of date added

It also makes processing a bit more complex than it should be. Canonical form for URL with bare domain...

has NO trailing slash when DISPLAYED in Firefox and Chrome UI

HAS trailing slash when COPIED from Firefox and Chrome UI

IIRC, we already had this discussion on canonical form of bare-domain URL, so let's take that decision and suggest it as canonical form.

The list-lists.py does have checks to avoid duplicates inside of the same list based on the canonical representation of the URL. We consider the canonical representation to be that without the trailing slash: https://github.com/citizenlab/test-lists/blob/master/scripts/lint-lists.py#L138

Another issue with trailing / is that http://example.com/ and http://example.com are same URLs, but http://example.com/foo/ and http://example.com/foo are different ones, so that specific heuristics may be wrong.
Example: https://vigruzki.rkn.gov.ru/auto != https://vigruzki.rkn.gov.ru/auto/

Good point, yet from the perspective of duplicate detection it's pretty likely that the user wanted to test only one of those two representations and I have yet to come across a case in which both representations should be tested. When that will happen I guess we can improve the heuristic.

darkk · 2018-10-10T19:25:12Z

scripts/sync-test-lists.py

+                                  ' VALUES (%s, %s, %s, %s, %s, %s, %s)'
+                                  ' ON CONFLICT DO NOTHING RETURNING url_no',
+                                  (url, cat_no, country_no, date_added, source, notes, True))
+                    except:


One URL ~ one SQL request. If the code is run in HKG and db is in AMS, then it's 250ms per URL, that's ~2h8m of runtime to import all lists (30780 lines in lists/*). It sounds suboptimal.

How do you suggest doing it instead?

COPY to temporary table, do manipulations, INSERT into main table. Tell me if an example is useful.

Yes an example for this would be useful.

darkk · 2018-10-10T19:26:51Z

scripts/sync-test-lists.py

+        with pgconn, pgconn.cursor() as c:
+            c.execute('INSERT INTO sync_test_lists (executed_at, commit_hash)'
+                      ' VALUES (NOW(), %s)',
+                      (last_commit_hash, ))


write_sync_table() writes meaningful data in separate transaction. It looks like a bug to me and other functions also follow same pattern of strange usage of transactions.

Yeah this is a good point. I will see if I can do some refactoring to wrap things in transactions in a better way.

darkk · 2018-10-10T19:28:35Z

scripts/sync-test-lists.py

 import psycopg2
 import git

 TEST_LISTS_GIT_URL = 'https://github.com/citizenlab/test-lists/'
+COUNTRY_UTIL_URL = 'https://github.com/hellais/country-util'


Should we run code instead of pulling from personal repos?

I don't understand this question. That repo contains JSON data for country codes and country names.

darkk · 2018-10-10T19:30:22Z

scripts/sync-test-lists.py

+    executed_at TIMESTAMP WITH TIME ZONE,
+    commit_hash VARCHAR PRIMARY KEY
+);
+"""


IMHO, either all DDLs should be here or none of them (IMHO, second is easier if we have more than one app speaking to the DB). Having some subset of tables being DDLed here and other subset somewhere else looks strange to me.

The reason to have this in here was that I didn't want to have to write a new migration script for orchestra and also that this table is never used by the orchestra app, but is only used by the sync_test_lists script.

darkk · 2018-10-10T19:41:21Z

scripts/sync-test-lists.py

+                        c.execute('INSERT INTO urls (url, cat_no, country_no, date_added, source, notes, active)'
+                                  ' VALUES (%s, %s, %s, %s, %s, %s, %s)'
+                                  ' ON CONFLICT DO NOTHING RETURNING url_no',
+                                  (url, cat_no, country_no, date_added, source, notes, True))


IMHO, country_iso2 is natural PK for the data, isn't it? TBH, I don't quite understand why country_no is used instead of country_iso2. Also, I'm unsure if name and full_name should be stored in database or not. Are these strings ever going to be translated? If they are, they probably should not be fetched from DB.
Same logic applies to category codes as well.
On the other hand, these questions are not that important, that's just a suggestion for reflection.

Good point for country_iso2. I will see how tricky it is to do a schema migration on that table and if so do that.

WRT name and full_name, these are the canonical english names (full_name is the name according to iso, while name is the name according to cldr) for those countries and I think putting them in the DB is a reasonable.

I'm not 100% sure if country_iso2 is a good point or not. We have cis.csv, you also add EU preudo-country somewhere...
I agree that putting names in the DB is reasonable for some use-cases. But we will eventually want to localize those names in UI, that's why I say that fetching those names from DB may be unreasonable in some cases. That's why I ask question if we have a use-case when we actually want to put a name into DB :)

darkk · 2018-10-10T19:44:56Z

scripts/sync-test-lists.py

@@ -309,18 +304,15 @@ def dirname(s):

 def parse_args():
    p = argparse.ArgumentParser(description='test-lists: perform operations related to test-list synchronization')
-    p.add_argument('--working-dir', metavar='DIR', type=dirname, help='where we should be cloning the git repository to', required=True)
-    p.add_argument('--init', action='store_true', help='if we should be running the initialisation routine')
+    p.add_argument('--working-dir', metavar='DIR', type=dirname, help='The working directory for this script. It should be preserved across runs.', required=True)


It's easier to avoid working-dir being cached and rely on git ls-remote https://github.com/citizenlab/test-lists/ HEAD to understand if we have to re-fetch or not.

I am not sure if gitpython supports this. Did you see it in their docs? Help on doing this would be useful.

Seems, it does not. subprocess.check_output with direct call to git cli or git.cmd is good enough.

Another random idea: we don't import URLs on commit-by-commit basis (do we want to?), so it may be easier to import all lists/* to temporary tables and act on "latest snapshot" and have a shallow clone instead of full clone...

darkk · 2018-10-12T13:34:28Z

Okay, I'll add ls-remote and then we'll decide about packaging this stuff to run on top of airflow.
BTW, something is wrong with travis... I bet, it has nothing to do with this branch, but maybe it's worth looking.

hellais · 2018-10-12T13:38:15Z

BTW, something is wrong with travis... I bet, it has nothing to do with this branch, but maybe it's worth looking.

Yeah. It should be fixed in the latest commit. Seems related to this: golang/lint#397.

hellais

This looks reasonable to me

That's python2.7 as cStringIO is gone in 3.x :-)

hellais added 2 commits October 9, 2018 17:44

Improvements to the sync-test-lists script and do some debugging of it

9d345d9

Improve docs of the working-dir argument

95f8645

hellais changed the base branch from master to feature/signing October 9, 2018 15:48

hellais changed the base branch from feature/signing to master October 9, 2018 15:48

hellais mentioned this pull request Oct 9, 2018

Add sync-test-lists script to pipeline ooni/pipeline#123

Closed

Refactoring of the sync-test-lists script

d78bdc6

Write the last synched commit to the sync_test_lists table

darkk reviewed Oct 10, 2018

View reviewed changes

hellais added 5 commits October 11, 2018 11:50

Wrapp all DB operations in transactions

09e42b2

Fix typo in cursor

e76b4aa

Avoid doing too many inserts inside of a transaction

b2318f6

Do the first INSERT in a copy_from

73d8b0a

Add bad_chars check

53619f2

Fix installation of golint

661528a

hellais and others added 3 commits October 17, 2018 12:07

Fix bug in sync-test-lists

f4adcc5

Capitalize globals, fix unused / undefined variables

993cd5d

Use single PG connections, check git ls-remote for possible fast track

d447a4a

hellais commented Oct 17, 2018

View reviewed changes

Dockerize sync-test-lists.py

1f800a5

That's python2.7 as cStringIO is gone in 3.x :-)

hellais merged commit 798f07f into master Oct 22, 2018

bassosimone deleted the sync-test-lists branch December 16, 2019 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync test lists #50

Sync test lists #50

hellais commented Oct 9, 2018 •

edited

Loading

darkk Oct 10, 2018

hellais Oct 11, 2018

darkk Oct 11, 2018

hellais Oct 11, 2018

darkk Oct 11, 2018

hellais Oct 11, 2018

darkk Oct 11, 2018 •

edited

Loading

hellais Oct 15, 2018

darkk Oct 17, 2018

hellais Oct 17, 2018

darkk Oct 10, 2018

hellais Oct 11, 2018

darkk Oct 11, 2018 •

edited

Loading

hellais Oct 11, 2018

darkk Oct 10, 2018

hellais Oct 11, 2018

darkk Oct 10, 2018

hellais Oct 11, 2018

darkk Oct 10, 2018

hellais Oct 11, 2018

darkk Oct 10, 2018 •

edited

Loading

hellais Oct 11, 2018

darkk Oct 11, 2018

darkk Oct 10, 2018 •

edited

Loading

hellais Oct 11, 2018

darkk Oct 11, 2018

darkk commented Oct 12, 2018

hellais commented Oct 12, 2018

hellais left a comment

Sync test lists #50

Sync test lists #50

Conversation

hellais commented Oct 9, 2018 • 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

darkk Oct 11, 2018 • 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

darkk Oct 11, 2018 • 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

darkk Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkk Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkk commented Oct 12, 2018

hellais commented Oct 12, 2018

hellais left a comment

Choose a reason for hiding this comment

hellais commented Oct 9, 2018 •

edited

Loading

darkk Oct 11, 2018 •

edited

Loading

darkk Oct 11, 2018 •

edited

Loading

darkk Oct 10, 2018 •

edited

Loading

darkk Oct 10, 2018 •

edited

Loading