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 Repro-CI Artifacts Reflect Configs Repo Structure #74

Closed
wants to merge 9 commits into from

Conversation

CodeGat
Copy link
Collaborator

@CodeGat CodeGat commented Oct 9, 2024

The Problem

Currently, the struture of the checksums in a repro test experiment running on Gadi are the following:

$EXPERIMENT_LOCATION/
└── checksum/
    ├── historical-*hr-checksum.json
    └── test_report.xml

And the structure of the checksums in the repo are:

.
└── testing/
    └── checksum/
        ├── historical-*hr-checksum.json
        └── test_report.xml

In which, in the CI, we special-case the logic for creating the intermediate testing directory in the repo. This also creates problems for the inputs.additional-artifact-content-paths, which are, confusingly, in the correct place at both the experiment level and the repo level, which means they both have to be handled differently.

The Solution

This PR unifies the checksum directories so that both the experiment and repo reflect the following structure:

$EXPERIMENT_LOCATION_OR_REPO_ROOT/
└── testing/
    └── checksum/
        ├── historical-*hr-checksum.json
        └── test_report.xml

In this PR:

  • test-repro.yml: put checksums under ./testing directory in artifact and experiment to mimic default repo structure
  • generate-checksums.yml: put checksums under ./testing directory in artifact and experiment to mimic default repo structure
  • Updated the creation and usage of artifacts to account for the new ./testing directory for checksums

NOTE: There is a unhealthy amount of duplication of effort in the PR, especially in test-repro/generate-checksums (see 34b7697 and 37518f4). This will be rectified in #72.

NOTE: This is required to merge #73

Closes #75

@CodeGat CodeGat self-assigned this Oct 9, 2024
CodeGat added a commit that referenced this pull request Oct 9, 2024
@aidanheerdegen aidanheerdegen self-requested a review October 9, 2024 04:49
.github/workflows/config-pr-1-ci.yml Outdated Show resolved Hide resolved
.github/workflows/generate-checksums.yml Outdated Show resolved Hide resolved
Copy link
Member

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

I accidentally just commented directly, but I have a few questions.

@CodeGat
Copy link
Collaborator Author

CodeGat commented Oct 10, 2024

Alright, there's been a bit of change since the last review @aidanheerdegen - this is because I realised that in OM3s case, we needed to an additional artifact path that has docs prepended. Here are those changes in a more easily digestable format:

@CodeGat CodeGat marked this pull request as draft October 15, 2024 22:40
@CodeGat
Copy link
Collaborator Author

CodeGat commented Oct 17, 2024

Closing this for now as it deals with committing inputs.additional-artifact-content-paths which we don't want to enable just yet.
It still has a lot of good stuff relating to making the artifacts closer to the experiment, so I won't delete the branch just yet.

@CodeGat CodeGat closed this Oct 17, 2024
CodeGat added a commit that referenced this pull request Oct 17, 2024
* Add config-comment-repro.yml workflow for repro tests on any branch!

* Replace `against COMMITISH` with `compare COMMITISH`

* Added ability to commit the result of the repro test to the PR

* Added RUN_URL, removed some of the branch name stuff

* Add additional artifact content paths section for repro-ci

* Removed custom `[compare COMMITISH]` logic

* Update artifact structure to be in line with #74

* Renamed file, made it a more generic `!test` command

* Changed artifact paths back to initial version, removed `inputs.additional-artifact-content-paths` input

* Removed issue property from `github.event.comment.body`

* Reworked permission checking, bug fixes, comment updates

* Further changes from testing

* Added error-checking for `!test` command

* Made comments more informative

* Update README.md to account for new `!test repro` syntax

* Update .github/workflows/config-comment-test.yml

Co-authored-by: Aidan Heerdegen <[email protected]>

* README.md: Note the case-sensitivity of the command

* Add step in prepare-command to check the command starts with `!test`

---------

Co-authored-by: Aidan Heerdegen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify Repro CI Experiment Checksum structure with Config Repo structure
2 participants