Skip to content
This repository was archived by the owner on Aug 1, 2024. It is now read-only.

feat: add optional coursegraph service#820

Merged
kdmccormick merged 2 commits intomasterfrom
kdmccormick/coursegraph
Aug 18, 2021
Merged

feat: add optional coursegraph service#820
kdmccormick merged 2 commits intomasterfrom
kdmccormick/coursegraph

Conversation

@kdmccormick
Copy link
Copy Markdown
Contributor

@kdmccormick kdmccormick commented Aug 17, 2021

Description

Add a "coursegraph" service to devstack using
the official Neo4j Docker image.

The service is "extra"; that is, it will not start, be
pulled, or be provisioned by default. Instead,
provisioning can be done on-demand,
which will dump data from LMS to Coursegraph:

  make dev.provision.coursegraph

Commands like `dev.up.<service>`, `dev.attach.<service>`, etc
are available for coursegraph, as they are for any other
Devstack service. Further documentation will live in the
edx-platform repo, in:

  ./openedx/core/djangoapps/coursegraph/README.rst

Why add Coursegraph? It is a tool that is relied upon
by many edX developers and support specialists,
yet very few of us know how to operate it or fix it
when it goes down. By making it part of devstack,
it will be easier to debug issues and test out Neo4j
upgrades. Furthermore, this enables developers
to trial complex Cypher queries on custom data
instead of simply trusting what they see returned
by production Coursegraph.

TNL-8386

Supporting information

I needed to run Coursegraph locally in order to debug the issues that edX has been having with its Coursegraph instance (TNL-8386). The easiest way to do so was via the official Neo4j Docker image. At that point, I'd nearly added to devstack, so I figured I would make this PR to bring it all the way.

Supporting documentation is being added here: https://github.com/edx/edx-platform/pull/28489

Testing instructions

Follow the testing instructions of https://github.com/edx/edx-platform/pull/28489

Deadline

None

Other information

@kdmccormick kdmccormick force-pushed the kdmccormick/coursegraph branch from 57b85c2 to 517214a Compare August 17, 2021 21:41
@kdmccormick kdmccormick force-pushed the kdmccormick/coursegraph branch from 517214a to 0bd80b1 Compare August 18, 2021 16:18
@kdmccormick kdmccormick changed the title draft: feat: add optional coursegraph service feat: add optional coursegraph service Aug 18, 2021
@kdmccormick kdmccormick marked this pull request as ready for review August 18, 2021 16:18
Copy link
Copy Markdown
Contributor

@adzuci adzuci left a comment

Choose a reason for hiding this comment

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

I'm approving this PR with the caveat that I haven't actually tested launching or destroying coursegraph on my local devstack. If you feel that further testing is needed it may make sense to ask someone to walk through the steps to spin up and down coursgraph locally.

Comment thread Makefile
Comment thread README.rst Outdated
Comment thread docker-compose.yml
coursegraph:
container_name: "edx.${COMPOSE_PROJECT_NAME:-devstack}.coursegraph"
hostname: coursegraph.devstack.edx
# Try to keep this in sync with the NEO4J_VERSION declared within
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for adding this comment! I think it could be worth adding a reciprical comment to https://github.com/edx/configuration/blob/master/playbooks/roles/neo4j/defaults/main.yml#L25 as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will do 👍🏻

Comment thread docker-compose.yml Outdated
Add a "coursegraph" service to devstack using
the official Neo4j Docker image.

The service is "extra"; that is, it will not start, be
pulled, or be provisioned by default. Instead,
provisioning can be done on-demand,
which will dump data from LMS to Coursegraph:

  make dev.provision.coursegraph

Commands like `dev.up.<service>`, `dev.attach.<service>`, etc
are available for coursegraph, as they are for any other
Devstack service. Further documentation will live in the
edx-platform repo, in:

  ./openedx/core/djangoapps/coursegraph/README.rst

TNL-8386
@kdmccormick kdmccormick force-pushed the kdmccormick/coursegraph branch from 0bd80b1 to 1b15c99 Compare August 18, 2021 19:44
Copy link
Copy Markdown
Contributor

@doctoryes doctoryes left a comment

Choose a reason for hiding this comment

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

This addition is long overdue and a huge step towards us being able to maintain coursegraph. 👍🏼

Comment thread Makefile
Comment thread provision-coursegraph.sh

echo -e "${GREEN} Updating LMS courses in Coursegraph...${NC}"

docker-compose exec lms bash -c 'source /edx/app/edxapp/edxapp_env && cd /edx/app/edxapp/edx-platform/ && ./manage.py lms dump_to_neo4j --host coursegraph.devstack.edx --user neo4j --password edx'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the Makefile support a re-syncing command that's outside of the more extreme action of re-provisioning? Or is re-provisioning the prescribed way of re-syncing the LMS course data?

Copy link
Copy Markdown
Contributor Author

@kdmccormick kdmccormick Aug 18, 2021

Choose a reason for hiding this comment

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

@doctoryes yeah, I went back and forth between having a less-extreme command like make dev.sync-coursegraph, versus just stuffing it all into provisioning.

The advantage of the current setup is that it'll be trivial to push out updates to the coursegraph image, since folks will need to pull down the new image in order to sync data. The disadvantage is that it adds ~30 seconds to the syncing process.

What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess it depends on how frequently/quickly developers will want to re-sync the data. Going with this solution at first seems fine. If someone needs a tighter iteration loop, then it's no longer YAGNI and they can add it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I like that take 👍🏻

Copy link
Copy Markdown
Contributor

@doctoryes doctoryes left a comment

Choose a reason for hiding this comment

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

👍🏼

@kdmccormick kdmccormick merged commit 39571ca into master Aug 18, 2021
@kdmccormick kdmccormick deleted the kdmccormick/coursegraph branch August 18, 2021 21:03
nsprenkle pushed a commit that referenced this pull request Nov 21, 2023
Add a "coursegraph" service to devstack using
the official Neo4j Docker image.

The service is "extra"; that is, it will not start, be
pulled, or be provisioned by default. Instead,
provisioning can be done on-demand,
which will dump data from LMS to Coursegraph:

  make dev.provision.coursegraph

Commands like `dev.up.<service>`, `dev.attach.<service>`, etc
are available for coursegraph, as they are for any other
Devstack service. Further documentation will live in the
edx-platform repo, in:

  ./openedx/core/djangoapps/coursegraph/README.rst

Why add Coursegraph? It is a tool that is relied upon
by many edX developers and support specialists,
yet very few of us know how to operate it or fix it
when it goes down. By making it part of devstack,
it will be easier to debug issues and test out Neo4j
upgrades. Furthermore, this enables developers
to trial complex Cypher queries on custom data
instead of simply trusting what they see returned
by production Coursegraph.

TNL-8386
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants