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

Android Tweaks #4158

Merged
merged 9 commits into from
Feb 4, 2019
Merged

Android Tweaks #4158

merged 9 commits into from
Feb 4, 2019

Conversation

DXCanas
Copy link
Member

@DXCanas DXCanas commented Aug 13, 2018

@DXCanas
Copy link
Member Author

DXCanas commented Aug 21, 2018

@aronasorman what's left to make this ready to merge? I tried importing multiprocessing rather than the myhero dummy package, but I'm getting a bunch of flake8 errors.

@aronasorman
Copy link
Collaborator

I remember we needed to get space usage working properly before we can actually import content. But that can be a separate PR.

@indirectlylit indirectlylit added this to the 0.11.0 milestone Aug 23, 2018
@DXCanas
Copy link
Member Author

DXCanas commented Aug 28, 2018

@aronasorman ah you're right.

@jamalex I'm still trying to figure out some JNI issues with Kevin, but Aron and I have a hunch that the import page isn't working because of some space calculation stuff in Morango - was that what you were talking about when I first kicked off?

@indirectlylit indirectlylit removed this from the 0.11.0 milestone Sep 5, 2018
@indirectlylit
Copy link
Contributor

closing PR until it's time to revisit this work

@DXCanas DXCanas deleted the android-testing branch October 3, 2018 21:24
@DXCanas DXCanas restored the android-testing branch November 6, 2018 18:45
@DXCanas DXCanas reopened this Nov 6, 2018
@codecov
Copy link

codecov bot commented Nov 6, 2018

Codecov Report

Merging #4158 into develop will decrease coverage by 0.23%.
The diff coverage is 18.18%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4158      +/-   ##
===========================================
- Coverage     51.2%   50.97%   -0.24%     
===========================================
  Files          864      870       +6     
  Lines        26668    26799     +131     
  Branches      3552     3572      +20     
===========================================
+ Hits         13656    13660       +4     
- Misses       12268    12385     +117     
- Partials       744      754      +10
Impacted Files Coverage Δ
.../core/content/management/commands/importcontent.py 92.37% <ø> (-0.07%) ⬇️
kolibri/utils/system.py 30.35% <0%> (-3.31%) ⬇️
kolibri/core/tasks/management/commands/base.py 76.78% <100%> (+0.11%) ⬆️
kolibri/core/analytics/pskolibri/common.py 76.81% <100%> (+0.34%) ⬆️
kolibri/core/discovery/utils/filesystem/posix.py 62.37% <12.5%> (-4.65%) ⬇️
...ach/assets/src/modules/classSummary/dataHelpers.js 44% <0%> (-17.37%) ⬇️
...ins/coach/assets/src/modules/classSummary/index.js 40% <0%> (-1.94%) ⬇️
.../assets/src/views/reports/ReportsGroupListPage.vue 0% <0%> (ø) ⬆️
...h/assets/src/views/home/HomePage/OverviewBlock.vue 0% <0%> (ø) ⬆️
...ssets/src/views/reports/ReportsLearnerListPage.vue 0% <0%> (ø) ⬆️
... and 34 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 ef7f35c...3ae1202. Read the comment docs.

@DXCanas
Copy link
Member Author

DXCanas commented Nov 7, 2018

@jamalex @ralphiee22 The shim is effective in granting access to the import from studio workflow, but I get this error:

quickmemo _2018-11-06-16-56-34

When I try to open up a channel's page. Are file size reports a morango thing?

@indirectlylit
Copy link
Contributor

DO NOT ACKNOWLEDGE. I JUST NEED THE ARTIFACTS

Acknowledged

@DXCanas DXCanas added the work-in-progress Not ready for review label Nov 7, 2018
@jamalex
Copy link
Member

jamalex commented Nov 7, 2018

Are file size reports a morango thing?

No, they're calculated here:
https://github.com/learningequality/kolibri/blob/develop/kolibri/core/discovery/utils/filesystem/posix.py#L89

If you have a QPython shell, you could try some of those things interactively on an Android device.

(Note: this is about free disk space, not file sizes)

@DXCanas DXCanas changed the title IGNORE: Android fix tests. Shimming tqdm tests Android Tweaks Nov 13, 2018
kolibri/core/tqdm_shim.py Outdated Show resolved Hide resolved
kolibri/core/tqdm_shim.py Outdated Show resolved Hide resolved
@DXCanas DXCanas added TODO: needs review Waiting for review and removed work-in-progress Not ready for review labels Jan 16, 2019
Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

I haven't tested, but nothing in here looks problematic to me

kolibri/core/discovery/utils/filesystem/posix.py Outdated Show resolved Hide resolved
@benjaoming
Copy link
Contributor

This is targeted for 0.11.x, but the tqdm removal was targeted for 0.12?

@indirectlylit
Copy link
Contributor

this can be retargeted to 0.12

@DXCanas
Copy link
Member Author

DXCanas commented Feb 1, 2019 via email

@indirectlylit
Copy link
Contributor

originally the goal was to release with 0.11 but that boat sailed, so we can just target 0.12 now

@benjaoming
Copy link
Contributor

Thanks :) I was also wondering, but given the motivation behind the tqdm removal, I hope we can get this in for 0.12 👍

@indirectlylit
Copy link
Contributor

Yeah I approved it a while ago - @DXCanas are you still working on it?

@indirectlylit indirectlylit changed the base branch from release-v0.11.x to develop February 1, 2019 18:49
@indirectlylit
Copy link
Contributor

changed target to develop which introduced conflict in base.txt

@DXCanas
Copy link
Member Author

DXCanas commented Feb 1, 2019

Not working on it anymore. I'll handle the conflict then merge?

@DXCanas
Copy link
Member Author

DXCanas commented Feb 1, 2019

@indirectlylit rebased. Figured it would be a cleaner merge that way

@benjaoming
Copy link
Contributor

18.18% of diff hit (target 51.2%)

Is there a way @DXCanas that this can be tested, or is it too Android-specific? Some mocks perhaps?

@indirectlylit
Copy link
Contributor

good call @benjaoming -

I opened a new issue and targeted it to 0.12.1 so this doesn't become a blocker: #4907

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... OS: Android TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants