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

Make test cases automatically run using github actions #13

Closed
vincepick opened this issue Nov 13, 2024 · 16 comments
Closed

Make test cases automatically run using github actions #13

vincepick opened this issue Nov 13, 2024 · 16 comments
Assignees
Labels
enhancement New feature or request

Comments

@vincepick
Copy link
Collaborator

Make the existing test run automatically on each pull request.

@vincepick vincepick added the enhancement New feature or request label Nov 13, 2024
@vincepick vincepick self-assigned this Nov 13, 2024
@vincepick
Copy link
Collaborator Author

The action itself has been created, and the tentative version can be seen at .github/workflows/main.yaml in the test/macc-gradedCase branch. To properly run the test, the best course of action will be to use a Docker container to run the tests in, as a result, this is effectively blocked until I finish implementing the Docker container fully #7 .

@vincepick
Copy link
Collaborator Author

vincepick commented Dec 10, 2024

No longer blocked by docker image, but currently blocked due to issue with the needed Conjure image version not pulling correctly: conjure-cp/conjure#676

Other than this block, seems to work correctly.

@vincepick
Copy link
Collaborator Author

vincepick commented Dec 12, 2024

Tests passing correctly, may need further configuration to migrate between AutoIG fork and main Repo correctly, but otherwise the code is fully finished.

Will leave the PR as a draft until this issue: conjure-cp/conjure#676 is resolved as well to use best practice of having a specific Conjure base image.

Image of initial test Suite passing.
image

Image of workflow running:
image

@vincepick
Copy link
Collaborator Author

The action seems to work, but sometimes has issues with downloading the savilerow .tgz file. The file is acessible for me on my local machine, and the workflow seems to pass or fail sometimes as a result of this download only working sometimes. Perhaps the workflow has been run too many times in quick succession recently, and the source website is denying the runner's attempt to access it? Not sure if this will continue to be an issue, but will keep an eye on it for the future.

@ozgurakgun
Copy link

Why do you need that file? Conjure repo comes with a jar file already?

@vincepick
Copy link
Collaborator Author

I've been working to ensure that AutoIG is backward compatible and can still be set up in Linux without the use of the container. So this is in the case of it being setup like it used to, where it downloads savilerow through the script bin/install-savilerow.sh, rather than using the instance in .local/bin when run in a container on top of the Conjure base image.

@ozgurakgun
Copy link

why would using the one in the repo be less backward compatible? I think it's just easier, unless I am missing something.

@vincepick
Copy link
Collaborator Author

vincepick commented Dec 19, 2024

I'm not entirely sure what you mean, by the one in the repo do you mean the version currently in Main (run without container)? When its run in a container without redundant installations (the way I set up and have been developing), one of the changes is a new path setting script to point toward some binaries in .local/bin that come with the Conjure base image, rather than only to ones in AutoIG's own bin. The existing path setting script which is still used without a container only points to things in AutoIG's own bin.

Backward compatible may have been the wrong way of putting it, I have just been working to make sure that even while adding these new scripts it still works without the container, and thats how the pipeline currently operates. This method works for testing all the source code, but won't be able to test that the container itself is made correctly automatically. This is why I want to eventually make another pipeline that builds the container and runs tests in it directly.

This isn't directly related to this specific issue, but just to be clear about the path setup using the container, its worth noting that AutoIG installs some things as a bundle which come with other solvers/programs that the Conjure installation dont. As a result, in the container version I've made it so that some of the instals into .local/bin are deleted and had Conjure use a differently installed one from a bundle in the AutoIG bin. This was done to keep the image size as small as possible, while also making sure AutoIG has everything it needs. This approach seems to work well and is passing all my tests and Nguyen said it worked. If you have any suggestions though I would be happy to hear them, I have very little experience with containers and trust your judgement much more than mine : ).

For example with MiniZinc, AutoIG downloads this bundle which comes with numerous other things:
https://github.com/MiniZinc/MiniZincIDE/releases/tag/2.8.5

While conjure installs:
[[[/](https://github.com/google/or-tools)](https://github.com/google/or-tools)](https://github.com/MiniZinc/libminizinc.git)

@ozgurakgun
Copy link

You can overwrite whatever you want, like install a different bundle etc, but I would use what Conjure comes with. What additional things does AutoIG need btw? This is news to me.

Here is the far file in the repo: https://github.com/conjure-cp/conjure/tree/main/etc/savilerow

  1. If you don't want any docker images, make && make solvers will put conjure, sr, solvers in $BIN_DIR
  2. If you want Conjure in docker but AutoIG not in docker, just use docker run
  3. If you want to use the AutoID docker image built on top of Conjure, I think you know what to do?

Option (2) is quite sensible for me, since you will be changing AutoIG quite a lot but using a set version of Conjure for a while. Something like the following should work.

docker run --rm \
  -v "$(pwd)":/work \
  -w /work \
  ghcr.io/conjure-cp/conjure:WHATEVERVERSION \
  conjure solve x.essence -o output-dir

-v to mound the current dir as /work and -w to set the working directory. this is so x.essence and output-dir become available both in the host system and inside docker.

similarly you can docker run savilerow and that should work too since it's in the docker image.

@vincepick
Copy link
Collaborator Author

vincepick commented Dec 19, 2024

The minizinc bundle comes with other solvers included in it, which are directly used by AutoIG. I could have installed them seperately and added them to Conjure's download of MiniZinc, but didn't see the advantage, and Nguyen agreed as well when we met about it yesterday.

The MZN bundle comes with all these in its own bin, while the Conjure install seemed to just be the binary. The architecture of AutoIG already is made to expect this specific setup as well, so recreating the bundle again and adding it to the version Conjure uses would be quite a bit more complicated :
image

I'm a little confused about what it is you are suggesting.

The two current setups for AutoIG both function correctly, and are:

  1. The original, made before I was brought onto the project: AutoIG and Conjure both not using a container, Auto IG installing SR and Conjure into its own bin (the step currently breaking in the pipeline). This uses the existing AutoIG install: script
  2. The Container Approach Ive made, AutoIG is run in a container that is built off the Conjure base image. It uses the SR that comes with the conjure base image, but makes Conjure use the Minizinc instance it installs (into AutoIG/bin) because it comes bundled with other stuff that AutoIG uses. The MiniZinc binary it installs is the same thing and same version as the one Conjure would use to the best of my understanding, so there shouldn't be any issue with it having unexpected behavior with Conjure.

There is no scenario currently where it is used like your option 2, where Conjure is run in a docker but AutoIG is not, but it does seem to have some advantages and I would be happy to implement that as well at a later point, thanks for pointing it out.

@ozgurakgun
Copy link

Some of this sounds overly complicated to me, but I'll let you and @ndangtt decide what's best, of course. Just let me know if you need anything from me! I'll just say that just using what's in the Conjure image where possible makes sense to me, for example for SR.

Another option would indeed be to keep whatever other binaries you want to keep in https://github.com/conjure-cp/conjure/blob/d0721c192c1d63bb65b4aa8131d8fde4b101a3c9/etc/build/install-minizinc.sh#L21

@vincepick
Copy link
Collaborator Author

I tried a few other approaches as well. This one seemed to work the best, and needed the least refactoring of AutoIG itself. I will also discuss some of the considerations made in my Final Report.

Thank you for the quick feedback and your help throughout the semester, I'm going to keep these other options in mind for once we return from break.

Happy Holidays!

@ndangtt
Copy link
Contributor

ndangtt commented Dec 27, 2024

hi @vincepickm, hi @ozgurakgun! Sorry for being late at joining the discussion.

@vincepick: I agree that we can indeed use the SR that comes with Conjure. When we download conjure directly, SR is included anyway. Therefore, the install-savilerow.sh script can be removed. For other solvers that are also needed for minizinc experiments, I agree that it'd be best to install them using our installation script. @ozgurakgun: we made this decision after going through the installation together. It was quite tricky to make all the required installation for the minizinc experiments work using the ones provided in the conjure bundle, so we decided to install everything using our own scripts :)

If I understand it correctly, your suggestion on keeping what we need in the conjure image is another option indeed, but I suppose that'd mean we need to change the conjure image? For now I guess we'd re-use the conjure docker image and add things on top of that instead.

@ozgurakgun
Copy link

Just out of interest, why was it tricky? If something is not setup correctly for the conjure bundle we should fix it (ie I'd be happy to fix it)

@vincepick
Copy link
Collaborator Author

I don't think it is that Conjure is not set up correctly or needs a fix, just that AutoIG currently relies on other program installations that come with MiniZinc when it is installed from the release: https://github.com/MiniZinc/MiniZincIDE/releases/tag/2.8.5, while conjure is installed from: https://github.com/MiniZinc/libminizinc.git.

It was tricky because I initially set up the whole Docker, and installation scripts to point to the Conjure version of MiniZinc as planned, only to find that that version didn't actually have everything needed in the first place (I kept that version on a branch in case I ever need to refer back to it here: https://github.com/vincepick/AutoIG/tree/build/update-docker-preredoingredundant.

When installed through a release, MiniZinc comes with additional directories and files which are displayed here:
image

While the conjure installation comes with :
image

AutoIG uses the solvers directory and its contents in order to run. I could have made additional scripts to recreate it and its contained .msc files for each solver, but thought this would just add more setup scripts and complexity for no added benefits. In addition, the AutoIG MiniZinc installation comes with ortools and chuffed already installed, which is why I also remove them from the container image to save space in /update-conjure-paths.sh. Conjure could theoretically be updated to work with this bundle as well, but if it is already set up not to I don't think there is any need to change it.

Here is the relevant part of the script where conjures version of chuffed, minizinc, and ortools are removed from the container image.
image

@ndangtt
Copy link
Contributor

ndangtt commented Dec 28, 2024

thanks, @vincepick, for the detailed info!

just to add: in principle we could add the .msc file for each solver ourselves, but we need to point to the library of each solver, which wasn't included in the conjure bundle (for chuffed and ortools). When we install each solver ourselves, we can get those library files and add them to the share/minizinc/ folder directly.

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

No branches or pull requests

3 participants