Only run cache upstream ws if no .repos file changed in PRs by JafarAbdi · Pull Request #674 · moveit/moveit2

JafarAbdi · 2021-09-10T12:49:04Z

Description

One issue I had in this PR is a conflict between one of the cached upstream packages and the newly added hash for it which was causing the CI to always fail with the following error during vcs pull $BASEDIR/upstream_ws/src

  === /home/runner/work/moveit2/moveit2/.work/upstream_ws/src/moveit_resources (git) ===
  
  Already on 'pr-moveit_configs_utils'
  Your branch and 'origin/pr-moveit_configs_utils' have diverged,
  and have 1 and 3 different commits each, respectively.
    (use "git pull" to merge the remote branch into yours)

This will only affect PRs

Checklist

Required by CI: Code is auto formatted using clang-format
Extend the tutorials / documentation reference
Document API changes relevant to the user in the MIGRATION.md notes
Create tests, which fail without this PR reference
Include a screenshot if changing a GUI
While waiting for someone to review your request, please help review another open pull request to support the maintainers

vatanaksoytezer

Thanks a ton for fixing that @JafarAbdi! I saw the issue but I can't think of a better solution right now. This looks clean and neat.

codecov · 2021-09-10T14:04:06Z

Codecov Report

Merging #674 (54ad9bb) into main (f082f0a) will increase coverage by 0.14%.
The diff coverage is n/a.

❗ Current head 54ad9bb differs from pull request most recent head df23eca. Consider uploading reports for the commit df23eca to get more accurate results

@@            Coverage Diff             @@
##             main     #674      +/-   ##
==========================================
+ Coverage   54.24%   54.38%   +0.14%     
==========================================
  Files         192      191       -1     
  Lines       20230    20100     -130     
==========================================
- Hits        10972    10929      -43     
+ Misses       9258     9171      -87

Impacted Files	Coverage Δ
...eit_core/robot_trajectory/src/robot_trajectory.cpp	`15.75% <0.00%> (-7.38%)`	⬇️
...bot_state/include/moveit/robot_state/robot_state.h	`88.89% <0.00%> (-1.05%)`	⬇️
moveit_ros/moveit_servo/src/servo_calcs.cpp	`70.41% <0.00%> (-0.56%)`	⬇️
moveit_ros/moveit_servo/src/servo_parameters.cpp	`69.15% <0.00%> (-0.32%)`	⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp	`45.49% <0.00%> (-0.32%)`	⬇️
moveit_ros/moveit_servo/src/collision_check.cpp	`85.72% <0.00%> (-0.25%)`	⬇️
...anning_scene_monitor/src/current_state_monitor.cpp	`74.18% <0.00%> (-0.22%)`	⬇️
moveit_ros/moveit_servo/src/pose_tracking.cpp	`77.66% <0.00%> (-0.12%)`	⬇️
...ccupancy_map_monitor/src/occupancy_map_monitor.cpp	`18.90% <0.00%> (ø)`
...veit_servo/include/moveit_servo/servo_parameters.h	`69.24% <0.00%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f082f0a...df23eca. Read the comment docs.

rhaschke

Hm. Looks like the UPSTREAM_WORKSPACE is not correctly considered in CACHE_PREFIX. I will file a simpler solution.
The currently proposed solution would disable the cache once a change is detected.

rhaschke · 2021-09-15T09:59:25Z

I pushed a simpler fixup. However, unfortunately, the calculation of CACHE_PREFIX via hashFiles() doesn't work.
On my test branch https://github.com/rhaschke/moveit2/tree/pr-fix_ci I pushed some changes which should or should not invalidate the cache. However, it happens exactly the opposite of the expected:
When a file that is not hashed has changed, the target_ws cache was invalid (but upstream_ws cache was valid as expected).
When a file that is hashed has changed, the upstream_ws cache remains valid.

@vatanaksoytezer, could you have a look at this in a smaller toy example?
Maybe GHA cannot handle the (output) variable being used in hashFiles()?

JafarAbdi · 2021-09-15T12:17:23Z

The currently proposed solution would disable the cache once a change is detected.

This is exactly what I was trying to do, the problem we have is when someone opens a PR and change one of the repo's branch in .repos to point to a temporary branch (a PR in one of the upstream repos) and force push to that temporary branch, it will cause vcs pull to fail in some cases

The ultimate solution would be to always force pull but that's not possible in vcs right now

EDIT: force pulling didn't work :/ JafarAbdi#18

rhaschke · 2021-09-15T14:13:35Z

The currently proposed solution would disable the cache once a change is detected.

The point is that the cache wouldn't be rewritten as well. So it stays disabled forever!
We really need to fix the cache keys.

JafarAbdi · 2021-09-15T20:02:24Z

The point is that the cache wouldn't be rewritten as well. So it stays disabled forever!

Yes, it will be disabled forever only for the PRs that have changes to .repos

We really need to fix the cache keys.

I don't think this will help, in my case I pushed a conflicting change to the branch I used in the temporary repo way after changing the .repos file and it caused a problem once I pushed a new commit to the PR

vatanaksoytezer · 2021-09-15T20:08:20Z

@vatanaksoytezer, could you have a look at this in a smaller toy example?
Maybe GHA cannot handle the (output) variable being used in hashFiles()?

Sure, I'll make a minimal example and see what I can do.

rhaschke · 2021-09-15T20:11:27Z

Yes, it will be disabled forever only for the PRs that have changes to .repos

It will also be disabled as soon as we want to push a change in the .repos files to the main branch!
I don't get why you think that fixing the cache-key mechanism will not work. It's exactly intended for this purpose.

JafarAbdi · 2021-09-15T20:32:32Z

It will also be disabled as soon as we want to push a change in the .repos files to the main branch!

I don't think this will be the case, this only work for pull-request, the expression inside the if-statement will evaluate to false for the branches, I even tried changing it on my fork see

I don't get why you think that fixing the cache-key mechanism will not work. It's exactly intended for this purpose.

If we can get the cache-key solution to work it will be better than the original one, I'll meet with Vatan to work on fixing this

Looks like any kind of computed values for list of files is not considered at all. That is, the following tests failed: - hashFiles(steps.config.outputs.FILES) - hashFiles(fromJSON(steps.config.outputs.FILES))

rhaschke · 2021-09-16T06:39:52Z

I performed a series of tests in this toy repo. Looks like hashFiles doesn't support any kind of computed input list. Thus falling back to static list with wildcards. That's a little bit too broad, but these files don't change that often...

vatanaksoytezer · 2021-10-20T13:53:38Z

@rhaschke are we good to merge this? If so I'll start removing hashFiles from other repos as well so we don't hit this issue again.

vatanaksoytezer · 2021-10-20T15:54:27Z

Thanks @rhaschke!

Only run cache upstream ws if no .repos file changed in PRs

739e5e8

JafarAbdi requested review from rhaschke and vatanaksoytezer September 10, 2021 12:49

vatanaksoytezer approved these changes Sep 10, 2021

View reviewed changes

rhaschke reviewed Sep 15, 2021

View reviewed changes

Evaluate env.UPSTREAM_WORKSPACE for use in CACHE_PREFIX

33900d5

Revert to static list for hashFiles()

c1ae167

Looks like any kind of computed values for list of files is not considered at all. That is, the following tests failed: - hashFiles(steps.config.outputs.FILES) - hashFiles(fromJSON(steps.config.outputs.FILES))

Include moveit2.repos

54ad9bb

Merge branch 'main' into pr-fix_ci

df23eca

rhaschke approved these changes Oct 20, 2021

View reviewed changes

rhaschke merged commit ed63d6b into moveit:main Oct 20, 2021

Conversation

JafarAbdi commented Sep 10, 2021

Description

Checklist

Uh oh!

vatanaksoytezer left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rhaschke left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhaschke commented Sep 15, 2021

Uh oh!

JafarAbdi commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhaschke commented Sep 15, 2021

Uh oh!

JafarAbdi commented Sep 15, 2021

Uh oh!

vatanaksoytezer commented Sep 15, 2021

Uh oh!

rhaschke commented Sep 15, 2021

Uh oh!

JafarAbdi commented Sep 15, 2021

Uh oh!

rhaschke commented Sep 16, 2021

Uh oh!

vatanaksoytezer commented Oct 20, 2021

Uh oh!

vatanaksoytezer commented Oct 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Sep 10, 2021 •

edited

Loading

rhaschke left a comment •

edited

Loading

JafarAbdi commented Sep 15, 2021 •

edited

Loading