(WIP) Crowdsource Hinter Prototype

[solashirai]

A prototype of the Crowdsource Hinter xblock. The hinter is an xblock that serves to provide hints for text/numerical input problems. The students themselves will populate a database of hints that correspond to specific mistakes on a problem.

There are many changes to the hinter that should probably be addressed - such as editing hints and categorizing hints for mistakes that are similar - but for now I am aiming for at least a test release of a very basic version of the hinter.

Acceptance and unit tests are included in the commit, but the acceptance test currently seems to not consistently succeed. I'm thinking that the tests' coverage also needs to be improved.

This feature has been developed under the direction of @pmitros .

[jbarciauskas]

Hey @solashirai, thanks for all this work! We're looking forward to working with you to make this a successful and productive contribution to the edX platform. Piotr, Sarina and I will get together and let you know exactly what the right steps are to ensure we are properly handling the IP related to this pull request. We'll let you know as soon as we've worked out the details.

Thanks again!

[solashirai]

@sarina @jbarciauskas Hello, I heard that pmitros will be unavailable for a while. Is there any update on how I should proceed with my pull request at the moment?

[pmitros]

Ali Mohammed has agreed to help shepherd this through (in my stead). If someone knows his github username, please tag him. He either has a lot more context, or where he doesn't, I can bring him up to speed.

[sarina]

@alawibaba - can you please help review this?

[openedx-webhooks]

Thanks for the pull request, @solashirai! I've created OSPR-744 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• edx-code email threads
• timeline information ('this must be merged by XX date', and why that is)
• partner information ('this is a course on edx.org')
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here.

We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation and added yourself to the AUTHORS file. Please see the CONTRIBUTING file for more information.

[sarina]

@pmitros is this an experimental xblock? If so it should perhaps go into requirements/edx/edx-private.

@solashirai regardless of where this is installed, we'd like to move to installing from github using tags - or ideally, installing from PyPI. Tags are easier, though :) the reason is that commits can get removed accidentally, but tags are persistent. See https://git-scm.com/book/en/v2/Git-Basics-Tagging

[pmitros]

@sarina Interesting choice. Tags would present a security issue. We could have unreviewed malicious code installed on our systems. If there were a malicious developer, or the machine of any developer of any code referenced by a tag were compromised, we'd have malicious code on edX.org. SHA hashes are secure -- you know what you're getting. @e0d

[feanil]

@pmitros since we are using github as our canonical source for tags, a malicious user would need to have access to the repo. At which point they could also remove the hash from the repo resulting in failures. The reason for moving to tags instead of hashes has been due to the fact that when hashes are used that have not been merged into master, they have been garbage collected or rebased out of existence(This has already happened multiple times). So we get ephemeral behavior where requirements work sometimes and then eventually break for fresh installs by groups in the community.

[pmitros]

a malicious user would need to have access to the repo

That's absolutely correct. Right now, we will do a review of Sola's code (or the RecommenderXBlock or ...). It is known to not contain back doors when it merges. Whenever we upgrade the hash, someone looks at the delta.

If we switched to tags, a malicious user would just need to compromise the account of any developer who has push access to any repo included in any of our requirements files to 0wn our servers. That's a very large security perimeter.

If a hash is rebased or garbage collected out of existence, the behavior we should see is a build a failure. This is a good behavior. If the code we want to deploy no longer exists, the servers should not build. With tags, we see a phantom change of code versions on our servers. It still builds, but it behaves differently, and possibly incorrectly. There's basically no way to debug or trace down why. That's a very bad behavior. @e0d

[nedbat]

My understanding is that a tagged commit will never be garbage collected. Perhaps the best approach is to tag the commit, then use the hash in the requirements file?

[pmitros]

@nedbat That sounds very reasonable.

[solashirai]

I think that I added a tag to my commit then pushed... should there be any change I can see within github? Also is there some kind of tagging convention that I should follow for this?

[sarina]

@pmitros is the intention for this to be used by course staff? If so it will need documentation.

If this is an experiment I recommend installing it in https://github.com/edx/edx-platform/blob/master/requirements/edx/edx-private.txt

[pmitros]

@sarina This is initially experimental. We do not want documentation or broad deployment in the prototype stage. However, a major reason to get this in are test cases. This is using several edge cases of XBlocks, which are otherwise untested, and we don't want those breaking. In edx-private, it will break the test suite for everyone who doesn't install edx-private.

[sarina]

@solashirai - We've talked about this with our legal team and we will need you to fill out a contributor's agreement. The agreement is here: http://open.edx.org/sites/default/files/wysiwyg/individual-contributor-agreement.pdf - you'll need to sign it and email it to the address indicated on the form. Thanks!

[nedbat]

@solashirai @pmitros We have a few concerns:

1. I understand the philosophy of testing edx-platform by exercising it with third-party components, but many people here are uncomfortable with relying on that third-party component for testing. Piotr mentioned a particular interaction that these tests exercised in particular. If we could come up with a way to test those natively, that would be ideal.

2. We do not want to install this XBlock into every Open edX instance. We don't currently have a mechanism for installing code solely for testing, and not into production, so we are a bit stuck with how to use crowdsourcehinter as a test dependency.

3. The analytics team is uncertain of the future of Logger.listen. I'm not sure what to do about that. The only code in edx-platform that uses it is an earlier version of crowdsourcehinter.

4. Speaking of the earlier version of crowdsourcehinter, what is its future?

[pmitros]

@nedbat

1. I'll pass on (1) to take it off-line. I think that's a continuation of our hallway discussion.

2. I think we do. This is a feature Anant would like to see as a core edx.org platform feature. This is a prototype, but it is designed to evolve into the final feature.

3. We had the Logger.listen discussion about a year ago. If we do deprecate it, we will want an alternative mechanism, and we will want to know code relying on it breaks.

4. Dead.

[pmitros]

@solashirai I spoke to @nedbat. He's not comfortable with the location of the tests, but willing to leave it if we don't figure out a better place of them at the point we merge. That should be an easy thing to move in the future if necessary, and the problem isn't isolated to this block. He'll work on that in parallel, and it's a question of who gets there first.

@nedbat I spoke to @explorerleslie. She said she was okay with two reviewers (e.g. myself and @alawibaba, if I can pursuade him to do it as a personal favor). If I cannot, she will find someone in T&L. We also need an accessibility review. I spoke with @cptvitamin, who said he could get to it late next week. Mark/Sola: You two should coordinate by email, since Mark usually wants to see a demo.

More detail on the points you raised (1) This is not intended to be external code in the long term. It was prototyped in edX by Felix. Sola's goal was to bring it to a production-ready state. This is not an external component for testing. This is an internal, prototype component. The tests let us know if the APIs which rely on it change. That would be possible to build independent tests for, but hard and actually less useful. If Logger goes away, we won't know what relies on it. To check if event formats change, we'll need a ton of independent tests, and even so, we won't know which downstream breaks as a result if there is an intentional change. (2) This will eventually hit production. This is on the path there. It has a ways to go, but when something doesn't interfere with other code, it's good to merge early and merge often. (3) We discussed this a year ago, and it was considered a supported API at the time. We need something like this regardless. I agree this is not the best way to do this, and there are better ways to do this based on native JS features (https://github.com/pmitros/2013septhack/blob/master/jstrack/framework.html is a much nicer mechanism). If we ever clean this up, it's easy enough to either change the XBlock or provide backwards-compatibility. Either way, we want to know we're breaking something. (4) Worked exceptionally well. It's the inspiration for this project. Unfortunately, it's an XModule, and the code is, for a number of reasons, not very portable or maintainable for edX circa. 2015.

@solashirai Apologies about all of the delays. I hope we can get this merged soon.

[nedbat]

I've made another pull request that creates the "openedx/tests/external" directory for tests like this. The tests in this PR should move there, once that PR merges: #9494. The bokchoy page objects defined in this PR could move into the crowdsourcehinter repo, but I think it's also ok to have them here.

We aren't going to change Logger.listen now, so this code should continue to use it.

Can the crowdsourcehinter in edx-platform be removed?

[solashirai]

@nedbat So should I close this PR?

[nedbat]

@solashirai No, no need to close this PR. You can continue to make changes, and push those changes to this branch, and the PR will be updated.

[nedbat]

@solashirai I've merged my other pull request into master. There is now a directory called "openedx/tests/xblock_integration". The tests in this pull request should be moved there.

[solashirai]

@nedbat I see where I should put my lms/djangoapps/courseware/tests/test_crowdsource_hinter.py file in the new directory. What should I do for the two acceptance test files?

[nedbat]

I would say all three of the testing files can go in that directory. I guess the page object should be in a subdirectory called "pages".

[nedbat]

@solashirai I'm a little confused where the test running stands. Ben suggested a location for the test, and it works, but you were unsure if it was ok to put it there?

[solashirai]

@nedbat Yes. I thought that previously there was some issues with having my testing files in directories other than the openedx/testing/, I thought perhaps I should wait for a response before pushing the change. Sorry, this fell off my radar after my second semester of college started.
I'm in the middle of fixing some issues with my computer but I can push the directory change for the bok_choy tests over the weekend.

[nedbat]

@solashirai this is almost ready to merge. It needs to be rebased against master, and the line in github.txt should change to this:

git+https://github.com/solashirai/crowdsourcehinter.git@518605f0a95190949fe77bd39158450639e2e1dc#egg=crowdsourcehinter-xblock==0.1

[solashirai]

@nedbat I ran a rebase and made the change you mentioned.

There's a bokchoy test failure, but my tests seem to be passing (test failure is in a video module). Looking into why that test is failing.

[nedbat]

jenkins run bokchoy because https://build.testeng.edx.org/job/edx-platform-bok-choy-pr/16183/

[nedbat]

@solashirai Thanks, and congratulations. Sorry it's been such a slow process :)