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

ci(build-and-test-differential): remove sync for the build-and-test-differential workflows #5200

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

xmfcx
Copy link
Contributor

@xmfcx xmfcx commented Oct 2, 2023

Description

Directly related sync job:

Proposal

To me, with the latest changes introduced by us, the sync job has grown way too much in complexity to be easily understood by anyone.
I think it is time to cut off the sync job at least for the build-and-test-differential.yaml workflow.

At least until we find a better way to sync this job. Or maybe once we make the containers smaller, we can remove the self hosted option too.

Function of this sync job

It syncs from:

But while syncing, it makes some brittle changes to the original file as instructed in the sync job file:

ChatGPT analysis of the current sync job

According to ChatGPT analysis, the rename operations can be summarized as follows:

The pre-commands in the second YAML file are using sd (substitute) command, which is a tool used for substituting or replacing strings in files, somewhat similar to sed command in Unix.

  1. First Command:

    sd "container: ros:(\w+)" "container: ghcr.io/autowarefoundation/autoware-universe:\$1-latest" {source}

    It substitutes all occurrences of container: ros:<any_word_character> with container: ghcr.io/autowarefoundation/autoware-universe:<any_word_character>-latest.
    Here <any_word_character> is captured by (\w+) and replaced by \$1.
    This change is applied to the {source}, referring to .github/workflows/build-and-test-differential.yaml.

  2. Second Command:

    sd -s 'container: ${{ matrix.container }}' 'container: ${{ matrix.container }}${{ matrix.container-suffix }}' {source}

    It substitutes the first occurrence of container: ${{ matrix.container }} with container: ${{ matrix.container }}${{ matrix.container-suffix }} in {source}, adding a potential suffix to the container matrix.

  3. Third Command:

    sd -- \
    "       include:" \
    "       container-suffix:
              - \"\"
              - -cuda
            include:" {source}

    This substitutes "include:" with a block that introduces a new matrix variable container-suffix with two values, an empty string, and -cuda, followed by the original include:.
    This likely enables running the workflow with and without CUDA support by appending the suffix to the container name.

  4. Fourth Command:

    sd "^    container: ghcr.io/autowarefoundation/autoware-universe:(\w+)-latest\$" \
        "    container: ghcr.io/autowarefoundation/autoware-universe:\$1-latest-cuda" {source}

    It substitutes all occurrences of container: ghcr.io/autowarefoundation/autoware-universe:<any_word_character>-latest at the beginning of a line with container: ghcr.io/autowarefoundation/autoware-universe:<any_word_character>-latest-cuda,
    effectively appending -cuda to the end of the container names.

In Summary:

These pre-commands are modifying the build-and-test-differential.yaml workflow's container names and introducing a new matrix variable,
likely to adjust the environment in which the jobs are run,
possibly to add support for CUDA and to use specific container images from ghcr.io/autowarefoundation/autoware-universe.

History of the sync job

Here is a brief history of the build-and-test-differential sync job in the Universe repository:

  1. The initial file was added to the sync job
  2. Some post-commands are added to rename the containers
  3. More rename operations are added to also have cuda containers too
  4. build_depends repos rename operations added
  5. pre-commands are renamed to post-commands and dest changed to src
  6. added no-cuda build with more complicated rename operations
  7. galactic replaced with a variable
  8. no-cuda test was disabled
  9. galactic ci was removed
  10. "no-cuda test was disabled" was reverted

And the manual changes made by me and @mitsudome-r :

Tests performed

Not applicable.

Effects on system behavior

Not applicable.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@github-actions github-actions bot added the type:ci Continuous Integration (CI) processes and testing. (auto-assigned) label Oct 2, 2023
@xmfcx xmfcx force-pushed the fix/ci-workflows branch from a5436d3 to 210a61e Compare October 2, 2023 10:00
@xmfcx xmfcx self-assigned this Oct 2, 2023
@xmfcx xmfcx requested a review from mitsudome-r October 2, 2023 10:19
@xmfcx xmfcx added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Oct 2, 2023
@xmfcx xmfcx requested a review from yukkysaito October 2, 2023 10:20
…ifferential workflows

Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
@xmfcx xmfcx force-pushed the fix/ci-workflows branch from 210a61e to 4cce03f Compare October 2, 2023 10:23
@xmfcx xmfcx changed the title fix(ci): remove sync for the build-and-test-differential workflows ci(build-and-test-differential): remove sync for the build-and-test-differential workflows Oct 2, 2023
@xmfcx xmfcx marked this pull request as ready for review October 2, 2023 10:23
@mitsudome-r
Copy link
Member

I agree that sync file job has become too complex.
I believe it was originally introduced to make sure that we have common settings among different Autoware related repositories so do you propose to maintain them individually manually?

I was just wondering if you want to find out a simpler way of maintaining similar files across different repositories, or you rather want to maintain them manually.

@xmfcx
Copy link
Contributor Author

xmfcx commented Oct 2, 2023

@mitsudome-r

I think we can maintain just the Universe one manually for now.

Once we merge this, there are 3 potential futures:

  1. Nothing changes
    • We manually maintain this single file.
  2. We don't need self hosted runners anymore because the containers are small again
    • We can revert back these changes.
  3. We somehow modify the parent file to have a "more easily modifiable" structure with conditional features maybe.
    • This way we can have many instances of this file with different feature sets enabled.
    • I'm not sure of the best way to achieve this just yet.
    • This would require us to modify all the packages that make use of the build-and-test-differential yaml files.

I'd like to go with option 1 for now since it requires the least effort.

Additional note on "self-hosted" naming scheme in many workflow files:

The build-and-test-differential-self-hosted doesn't make sense in the name anymore. All the current/old self-hosted things might need to be renamed to arm maybe.

Until now, we had all the arm things built in self hosted instances. But now we build x86 things in self hosted instances too.

But this will be so many refactorings all over the place, it is not high priority.

@mitsudome-r
Copy link
Member

mitsudome-r commented Oct 3, 2023

@xmfcx Thanks.
I'm okay with maintaining them manually.
We could change to use GitHub Runner if the cost is too high, but I think it's okay for now.

For the naming convention for "-self-hosted", you are right that we should probably rename them to "-arm", but I agree with you that it is low priority for now.

@xmfcx xmfcx merged commit 68544c7 into main Oct 3, 2023
@xmfcx xmfcx deleted the fix/ci-workflows branch October 3, 2023 09:13
@xmfcx xmfcx mentioned this pull request Oct 3, 2023
interimadd pushed a commit to interimadd/autoware.universe that referenced this pull request Mar 17, 2025
* fix devcontainer naming

Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>

* Update .devcontainer/cuda/devcontainer.json

Co-authored-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>

* Update .devcontainer/base/devcontainer.json

Co-authored-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>

---------

Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Co-authored-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:ci Continuous Integration (CI) processes and testing. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants