Retrieving en-minimal content pack + remove stale code by benjaoming · Pull Request #4865 · learningequality/ka-lite

benjaoming · 2016-02-18T14:30:53Z

Summary

Make the sdist backwards-compatible by shipping an English content db. After backwards-compat is asserted, remove all the stale code and JSON data.

This PR led me to fix unrelated issues:

Add better logging because I was finding it difficult to hunt down problems in JSON views
Update tests to match new content db
Fix an error in video download (because ultimately I needed to see that the full scope was working)
Hotfix (removing check total_files!=0) check because it was breaking the video update topic tree view

TODO

Update make assets
Merge in Preseed dist with content item db #4804 to use the English content db as a database template
Added proper logging so we can read everything in server.log in the same format (settings.LOGGING)
Remove stale code and data files (as @rtibbles had previously suggested but retargetted for 0.17)

Reviewer guidance

Check that there are no regressions in removed functionality.

Issues addressed

#4864
#3327 (logging)
#4719 (prettify unpacking assessment items)

Documentation

No

benjaoming · 2016-02-18T23:35:58Z

Functioning sdists now created with menus showing all the content items. Assessment items are available (as agreed theys hould be). However, video download menu is empty:

benjaoming · 2016-02-18T23:45:54Z

Clues welcomed, @aronasorman @rtibbles

rtibbles · 2016-02-18T23:47:11Z

Anything funny on the javascript console?

Also network tab output for update topictree API data would be handy.

On Thu, 18 Feb 2016, 15:45 Benjamin Bach notifications@github.com wrote:

Clues welcomed, @aronasorman https://github.com/aronasorman @rtibbles
https://github.com/rtibbles

—
Reply to this email directly or view it on GitHub
#4865 (comment)
.

benjaoming · 2016-02-18T23:48:47Z

Sorry, should've mentioned: No javascript errors! No log errors! Need advice on "output for update topictree API data" ?

rtibbles · 2016-02-18T23:51:44Z

API call to "/api/videos/topic_tree"

On Thu, 18 Feb 2016, 15:48 Benjamin Bach notifications@github.com wrote:

Sorry, should've mentioned: No javascript errors! No log errors! Need
advice on "output for update topictree API data" ?

—
Reply to this email directly or view it on GitHub
#4865 (comment)
.

rtibbles · 2016-02-18T23:53:19Z

Also, try running annotate_content_models if you have not already.

On Thu, 18 Feb 2016, 15:51 Richard Tibbles richard.tibbles@gmail.com
wrote:

API call to "/api/videos/topic_tree"

On Thu, 18 Feb 2016, 15:48 Benjamin Bach notifications@github.com wrote:

Sorry, should've mentioned: No javascript errors! No log errors! Need
advice on "output for update topictree API data" ?

—
Reply to this email directly or view it on GitHub
#4865 (comment)
.

benjaoming · 2016-02-19T00:02:37Z

annotate_content_items doesn't do the trick, it will update the database with availability. Just checked, all the videos are in the sqlite db.. just now showing up. Gonna look at API calls tomorrow.

rtibbles · 2016-02-19T00:11:02Z

The reason I ask I'd in case the "total_files" attribute is erroneously set
to 0, then things would not show up on update page, annotate_content_models
would rectify this.

On Thu, 18 Feb 2016, 16:02 Benjamin Bach notifications@github.com wrote:

annotate_content_items doesn't do the trick, it will update the database
with availability. Just checked, all the videos are in the sqlite db.. just
now showing up. Gonna look at API calls tomorrow.

—
Reply to this email directly or view it on GitHub
#4865 (comment)
.

benjaoming · 2016-02-19T00:13:09Z

Thanks! I'll make sure to have a look at that!

benjaoming · 2016-02-19T16:15:40Z

Yes, turns out total_files have to be set on both Topic and Video. Furthermore size_on_disk and remote_size is unset in en-minimal.zip

aronasorman · 2016-02-19T16:23:17Z

@benjaoming makes sense, we skip subtitles on en-minimal, and I think the file size stamping depends on that. Is that true @rtibbles?

rtibbles · 2016-02-19T16:24:33Z

Ah, @aronasorman might need to make a new version of the minimal zip, I
think he fixed this already.

On Fri, 19 Feb 2016, 08:15 Benjamin Bach notifications@github.com wrote:

Yes, turns out total_files have to be set on both Topic and Video.
Furthermore size_on_disk and remote_size is unset in en-minimal.zip

—
Reply to this email directly or view it on GitHub
#4865 (comment)
.

aronasorman · 2016-02-19T16:25:18Z

There's a couple of solutions we can do:

Instead of building en-minimal from scratch by passing --no-subtitles and --no-assessment-items in the content pack maker, we can instead just strip those folders from en.zip. That way we retain all the data in content db.
Make the file size stamping not depend on subtitles.

aronasorman · 2016-02-19T16:25:42Z

Did I? Heh, let me regenerate en.zip and en-minimal.zip and then upload it.

rtibbles · 2016-02-19T16:27:18Z

@aronasorman file size stamping is just part of the KA data fetch.

On Fri, 19 Feb 2016, 08:25 Aron Fyodor Asor notifications@github.com
wrote:

Did I? Heh, let me regenerate en.zip and en-minimal.zip and then upload
it.

—
Reply to this email directly or view it on GitHub
#4865 (comment)
.

benjaoming · 2016-02-19T16:28:45Z

Instead of building en-minimal from scratch by passing --no-subtitles and --no-assessment-items in the content pack maker, we can instead just strip those folders from en.zip. That way we retain all the data in content db.

You could universally do that for all languages!

Make the file size stamping not depend on subtitles.

That would be nice, also!

Let me know when it's uploaded, I'll have a look!

aronasorman · 2016-02-19T20:32:36Z

New en-minimal.zip file uploaded.

aronasorman · 2016-02-19T20:35:19Z

I installed en-minimal.zip, and all data should be there:

$ sqlite3 content_databases/content_khan_en.sqlite
SQLite version 3.8.2 2013-12-06 14:53:30
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> select total_files,remote_size,title from item where kind = "Video" limit 10;
1|1378133|Counting with small numbers
1|2489538|Counting in order
1|5377045|Number grid
1|3157982|Missing numbers between 0 and 120
1|4347537|Counting in pictures
1|1711467|Counting whales, sheep, and flowers
1|2533645|Counting dogs, mice, and cookies
1|3504210|Comparing numbers of objects
1|1169812|Comparing small numbers on the number line
1|2584625|Count by category

Yet, video download still doesn't show anything. I'll investigate why.

benjaoming · 2016-02-20T11:56:28Z

@aronasorman now it's also over 80 MB, why did it grow so drastically? It used to be just 20...

aronasorman · 2016-02-20T13:15:21Z

That's just in line in what it should be. Last content db did not include
everything, such as assessment items. Now it does!
On Feb 20, 2016 3:56 AM, "Benjamin Bach" notifications@github.com wrote:

@aronasorman https://github.com/aronasorman now it's also over 80 MB,
why did it grow so drastically? It used to be just 20...

—
Reply to this email directly or view it on GitHub
#4865 (comment)
.

benjaoming · 2016-02-20T13:38:15Z

I found the cause of why I can't download, will fix it asap...

ERROR:root:'module' object has no attribute 'CONTENT_DATABASE_ROOT'
Traceback (most recent call last):
  File "kalite/python-packages/fle_utils/videos.py", line 38, in download_video
    path, response = download_file(url, filepath, callback_percent_proxy(callback, end_percent=95))
  File "kalite/python-packages/fle_utils/internet/download.py", line 56, in download_file
    return urllib.urlretrieve(url, dst, lambda nb, bs, fs, url=url: callback(nb,bs,fs,url))
  File "/usr/lib/python2.7/urllib.py", line 94, in urlretrieve
    return _urlopener.retrieve(url, filename, reporthook, data)
  File "/usr/lib/python2.7/urllib.py", line 266, in retrieve
    reporthook(blocknum, bs, size)
  File "kalite/python-packages/fle_utils/internet/download.py", line 56, in <lambda>
    return urllib.urlretrieve(url, dst, lambda nb, bs, fs, url=url: callback(nb,bs,fs,url))
  File "kalite/python-packages/fle_utils/internet/download.py", line 30, in callback_percent_prox
y_inner_fn
    callback(start_percent + int(fraction * percent_range_size))
  File "kalite/updates/management/commands/videodownload.py", line 84, in download_progress_callback
    video_node = get_video_from_youtube_id(self.video.get("youtube_id"))
  File "kalite/topic_tools/content_models.py", line 429, in get_video_from_youtube_id
    for channel, language in available_content_databases():
  File "kalite/topic_tools/base.py", line 52, in available_content_databases
    for filename in glob.iglob(settings.CONTENT_DATABASE_ROOT):
AttributeError: 'module' object has no attribute 'CONTENT_DATABASE_ROOT'

benjaoming · 2016-02-20T16:18:35Z

@aronasorman my previous content.db also had assessment items but was 20 MB.. it looks like we jumped from 3,000 to 25,000 exercises?

benjaoming · 2016-02-20T16:23:00Z

There's still kind=Exercise in the item table (around 3,000 of those) and then the assessmentitem table with over 25,000 rows. Which part shouldn't be there? Or should they all be there?

Sign in to view


-		self.assertEqual(expected, actual)
+        expected = [u'counting-out-1-20-objects', u'counting-objects', u'one-more--one-less']
+        actual = get_exercises_from_topics(['cc-early-math-counting'])


Conflicts: Makefile

…/github.com/MCGallaspy/ka-lite into init_content_items_w_contentpack_minimal

Sign in to view

@@ -1,284 +0,0 @@
-import base


rtibbles · 2016-02-22T18:00:38Z

A few comments that need to be addressed before we can merge.

aronasorman · 2016-02-23T16:16:13Z

Aside from the video downloading having duplicates, is there anything else blocking this? If not, then can we merge this and open an issue for the video downloading?

benjaoming · 2016-02-23T16:23:41Z

Nothing is blocking this! All @rtibbles comments are addressed AFAIK. I want it merged asap, hence removed the [WIP] label :)

aronasorman · 2016-02-23T16:26:32Z

Cool, let's merge this, then please open an issue for the video download bug you were seeing!

…pack_minimal Retrieving en-minimal content pack + remove stale code

rtibbles · 2016-02-23T16:38:45Z

Not all my comments were addressed, the total_files filter has not been
reinstated.

On Tue, 23 Feb 2016, 08:26 Aron Fyodor Asor notifications@github.com
wrote:

Merged #4865 #4865.

—
Reply to this email directly or view it on GitHub
#4865 (comment).

benjaoming · 2016-02-23T16:41:54Z

@rtibbles but that's what I addressed, it's a hotfix for a separate issue that occurred as I'm trying to have a functional alpha. I commented on all you comments?

rtibbles · 2016-02-23T16:52:01Z

The content packs have the proper total files annotation now, and did
before this pr was merged. So, I didn't consider my comment addressed,
hence I had not merged it.

On Tue, 23 Feb 2016, 08:41 Benjamin Bach notifications@github.com wrote:

@rtibbles https://github.com/rtibbles but that's what I addressed, it's
a hotfix for a separate issue that occurred as I'm trying to have a
functional alpha. I commented on all you comments?

—
Reply to this email directly or view it on GitHub
#4865 (comment)
.

benjaoming · 2016-02-23T17:29:10Z

@rtibbles I think the rebuilt content packs overlapped with the communication, it's sorted out now.

MCGallaspy added 2 commits February 16, 2016 14:08

is_valid_url returns false when given windows path with drive letter

de3b20b

Unpack in separate thread, so that IO isn't blocked (it's nicer)

4458705

benjaoming added the bug label Feb 18, 2016

benjaoming added this to the 0.16.0 milestone Feb 18, 2016

benjaoming added the has PR label Feb 18, 2016

benjaoming mentioned this pull request Feb 18, 2016

Preseed dist with content item db #4804

Closed

move content item db location and distribute it

6791e6d

benjaoming reviewed Feb 21, 2016
View reviewed changes

benjaoming added 2 commits February 22, 2016 00:57

remove not-to-be-used generate_assessment_zips

e6d4de0

Merge branch '0.16.x' into init_content_items_w_contentpack_minimal

ca21814

Conflicts: Makefile

benjaoming mentioned this pull request Feb 22, 2016

0.16a2 #4882

Closed

8 tasks

Merge branch '4719-prettify-unpack-assessment-item-output' of https:/…

cd1adbc

…/github.com/MCGallaspy/ka-lite into init_content_items_w_contentpack_minimal

rtibbles reviewed Feb 22, 2016
View reviewed changes

Comment thread kalite/contentload/management/commands/channels/khan.py

@@ -1,284 +0,0 @@

import base

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This was referenced Feb 22, 2016

Log rotation #4890

Closed

0.16a3 #4891

Closed

Remove unnecessary steps from Makefile #4894

Closed

aronasorman added a commit that referenced this pull request Feb 23, 2016

Merge pull request #4865 from benjaoming/init_content_items_w_content…

f54b780

…pack_minimal Retrieving en-minimal content pack + remove stale code

aronasorman merged commit f54b780 into learningequality:0.16.x Feb 23, 2016

aronasorman removed the has PR label Feb 23, 2016

benjaoming deleted the init_content_items_w_contentpack_minimal branch May 2, 2016 15:22

benjaoming mentioned this pull request Sep 21, 2016

Run tests with language pack activated #4580

Closed

Uh oh!

Conversation

benjaoming commented Feb 18, 2016

Summary

TODO

Reviewer guidance

Issues addressed

Documentation

Uh oh!

benjaoming commented Feb 18, 2016

Uh oh!

benjaoming commented Feb 18, 2016

Uh oh!

rtibbles commented Feb 18, 2016

Uh oh!

benjaoming commented Feb 18, 2016

Uh oh!

rtibbles commented Feb 18, 2016

Uh oh!

rtibbles commented Feb 18, 2016

Uh oh!

benjaoming commented Feb 19, 2016

Uh oh!

rtibbles commented Feb 19, 2016

Uh oh!

benjaoming commented Feb 19, 2016

Uh oh!

benjaoming commented Feb 19, 2016

Uh oh!

aronasorman commented Feb 19, 2016

Uh oh!

rtibbles commented Feb 19, 2016

Uh oh!

aronasorman commented Feb 19, 2016

Uh oh!

aronasorman commented Feb 19, 2016

Uh oh!

rtibbles commented Feb 19, 2016

Uh oh!

benjaoming commented Feb 19, 2016

Uh oh!

aronasorman commented Feb 19, 2016

Uh oh!

aronasorman commented Feb 19, 2016

Uh oh!

benjaoming commented Feb 20, 2016

Uh oh!

aronasorman commented Feb 20, 2016

Uh oh!

benjaoming commented Feb 20, 2016

Uh oh!

benjaoming commented Feb 20, 2016

Uh oh!

benjaoming commented Feb 20, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

rtibbles commented Feb 22, 2016

Uh oh!

aronasorman commented Feb 23, 2016

Uh oh!

benjaoming commented Feb 23, 2016

Uh oh!

aronasorman commented Feb 23, 2016

Uh oh!

rtibbles commented Feb 23, 2016

Uh oh!

benjaoming commented Feb 23, 2016

Uh oh!

rtibbles commented Feb 23, 2016

Uh oh!

benjaoming commented Feb 23, 2016

Uh oh!

Reviewers