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

OctaveCodeQuestion redux #786

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

OctaveCodeQuestion redux #786

wants to merge 27 commits into from

Conversation

davis68
Copy link
Contributor

@davis68 davis68 commented Apr 7, 2021

This PR is a clean new build of OCQ on up-to-date RELATE. This will replace #633

@davis68
Copy link
Contributor Author

davis68 commented Apr 7, 2021

The major piece missing as of now is a good set of unit tests. Does the CI know to pull a new Docker image davis68/relate-octave?

@@ -376,7 +376,8 @@

# A string containing the image ID of the docker image to be used to run
# student Python code. Docker should download the image on first run.
RELATE_DOCKER_RUNPY_IMAGE = "inducer/relate-runcode-python"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have to revert this and the docker-build.sh image renaming.

@@ -73,6 +73,7 @@ def _skip_real_docker_test():
REAL_RELATE_DOCKER_URL = "unix:///var/run/docker.sock"
REAL_RELATE_DOCKER_TLS_CONFIG = None
REAL_RELATE_DOCKER_RUNPY_IMAGE = "inducer/relate-runcode-python"
REAL_RELATE_DOCKER_RUNOC_IMAGE = "davis68/relate-octave"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should break out a local image of davis68 as well.

@inducer
Copy link
Owner

inducer commented Apr 7, 2021

The major piece missing as of now is a good set of unit tests. Does the CI know to pull a new Docker image davis68/relate-octave?

You can just add a docker pull bit to the CI script.

@inducer
Copy link
Owner

inducer commented Apr 8, 2021

To cut down on noise in my email, I'm unsubscribing from this PR. When it next needs my attention, please @-mention me or hit the "request review" button. Otherwise, I may not see your messages in a timely manner.

@davis68
Copy link
Contributor Author

davis68 commented Apr 13, 2021

@inducer OK, this OctaveCodeQuestion seems to work in our basic testing. Please review and let us know. We used the test markdowns in test_code.py on the sandbox page to test robustness of the OCQ evaluation.

Thanks to @karthiksharma98 for testing with me.

@davis68
Copy link
Contributor Author

davis68 commented Apr 13, 2021

BEFORE MERGING I need to revert a couple of local settings that are swept up in here but those are minor.

@inducer
Copy link
Owner

inducer commented Apr 14, 2021

@davis68 There seem to be quite a few CI failures still, and these need to be fixed before merging. Would you be able to address those?

@davis68
Copy link
Contributor Author

davis68 commented Apr 21, 2021

@inducer, @karthiksharma98 and I have handled all of the linting and basic unit tests issues. Now there's a slew of connection-related issues on the page tests. Can you advise on those? These may be Docker/CI—I set it to pull davis68/relate-octave but it could be from that failing. (Local Docker is on the fritz, currently hacking on it.)

@inducer
Copy link
Owner

inducer commented Apr 21, 2021

An easy way to see what's going on in an actions run is to (temporarily) include this action in the failing job steps list in .github/workflows/ci.yml. Then you can SSH in and poke around to see why things are failing.

@davis68
Copy link
Contributor Author

davis68 commented Apr 22, 2021

I can see that no (?) Docker containers are starting and staying running for either Python or Octave. Since we didn't touch Python I have no idea why that would be the case.

@karthiksharma98
Copy link

@inducer Can you advise on this? We're able to get the markdowns in markdowns.py to work in the page sandbox on a local RELATE installation, but for CI it doesn't seem to work.

@davis68
Copy link
Contributor Author

davis68 commented Apr 7, 2022

@inducer do old PRs not trigger the CI tests?

@inducer
Copy link
Owner

inducer commented Apr 8, 2022

They should... I'm not sure what's wrong there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants