Skip to content

Conversation

@kevingurney
Copy link
Member

@kevingurney kevingurney commented Aug 25, 2022

Overview

This pull request:

  1. Removes hard-coded dependencies on "master" as the default branch name in the crossbow infrastructure and CI template files.

Implementation

  1. Removed comment/text references to "master" branch, including URLs to other repositories.
  2. Modified core.py to add a new default_branch property and a new method is_default_branch, for checking whether on the default branch, to the Target class.
  3. Modified CI template files to use the new is_default_branch function to check whether on the default branch.

Testing

  1. Using lafiona/crossbow as a queue repository for qualification.
  2. Ran modified template jobs. All failures appeared to be unrelated to the changes.
  3. The branch names for all relevant qualification jobs are prefixed with build-34-*.
  4. Example of a passing job: https://github.com/lafiona/crossbow/actions/runs/2920227769
  5. Example of a failing job: https://github.com/lafiona/crossbow/runs/7998190113 - in this example, the "Push Docker Image" workflow step is not included, since we are not on the default branch. The failure appears to be related to issues fetching R package resources and not related to the default branch checking logic. There were a variety of other kinds of failures, but none of them appear related to the default branch checking logic.

Future Directions

  1. Remove "master" from default_branch name property of Target class.
  2. Remove all remaining uses of "master" terminology in crossbow.
  3. ARROW-17512: Address minor issues with crossbow documentation.

Notes

  1. Thank you to @lafiona for her help with this pull request!
  2. Due to unexpected technical issues, we opened this pull request as a follow up to WIP: ARROW-15693: [Dev] Update crossbow templates to use master or main #13750. Please see WIP: ARROW-15693: [Dev] Update crossbow templates to use master or main #13750 for more discussion regarding qualification efforts.

… to other repositories.

2. Modified core.py to add default_branch property and a function forchecking whether on default branch, to Target.

3. Modified template files to use the python function for checking whether on default branch.

4. Updated macros.jinja to use python function for checking whether on default branch.

Co-Authored-By: Kevin Gurney <kgurney@mathworks.com>
@github-actions
Copy link

@kevingurney kevingurney changed the title WIP: ARROW-15693: [Dev] Update crossbow templates to use master or main ARROW-15693: [Dev] Update crossbow templates to use master or main Aug 29, 2022
@kevingurney kevingurney marked this pull request as ready for review August 29, 2022 15:24
@kevingurney
Copy link
Member Author

These changes should be ready for review now.

Our apologies for the delay - it took us some time to qualify our changes in a robust way using @lafiona's queue repository.

@assignUser - if you have any bandwidth to review these changes, that would be greatly appreciated. Thank you!

…om default_branch

property of Target class.

Co-authored-by: Fiona La <fionala7@gmail.com>
Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @kevingurney !
There are a couple of linting issues:

/arrow/dev/archery/archery/crossbow/core.py:744:80: E501 line too long (91 > 79 characters)
/arrow/dev/archery/archery/crossbow/core.py:784:9: E266 too many leading '#' for block comment
/arrow/dev/archery/archery/crossbow/core.py:784:80: E501 line too long (109 > 79 characters)

@kevingurney
Copy link
Member Author

kevingurney commented Aug 30, 2022

I'm very sorry @raulcd - I am not sure why I didn't notice the linting issues before! Thanks for pointing them out! I'll fix them as soon as possible.

…n from "is" to "in" after "master" is removed from the default_branch property.

2. Fix linting issues in core.py.

Co-authored-by: Fiona La <fionala7@gmail.com>
@kevingurney
Copy link
Member Author

kevingurney commented Aug 30, 2022

I just pushed a fix for the linting issues. My apologies again for not noticing them sooner!

Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

Thanks @kevingurney ! The PR looks good to me just a comment.

2. Add missing license header text.

Co-authored-by: Fiona La <fionala7@gmail.com>
Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me. @kszucs can you take a look on this one?

Copy link
Member

@kszucs kszucs left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @kevingurney !
Sorry for the late review, I was out.

@kszucs I think this should be mergable now?

@amol- amol- merged commit 3e40cd3 into apache:master Sep 6, 2022
@ursabot
Copy link

ursabot commented Sep 6, 2022

Benchmark runs are scheduled for baseline = 23c5cee and contender = 3e40cd3. 3e40cd3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed ⬇️1.37% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.53% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 3e40cd36 ec2-t3-xlarge-us-east-2
[Failed] 3e40cd36 test-mac-arm
[Failed] 3e40cd36 ursa-i9-9960x
[Finished] 3e40cd36 ursa-thinkcentre-m75q
[Finished] 23c5ceea ec2-t3-xlarge-us-east-2
[Failed] 23c5ceea test-mac-arm
[Failed] 23c5ceea ursa-i9-9960x
[Finished] 23c5ceea ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented Sep 6, 2022

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

@kevingurney
Copy link
Member Author

Thanks for the code reviews everyone!

Regarding the performance regressions reported by ursabot - these seem a bit surprising. My initial guess would be that they are unrelated to the infrastructural changes in this PR.

Perhaps, we should re-run the benchmarks to make sure these regressions aren't false positives.

@raulcd
Copy link
Member

raulcd commented Sep 6, 2022

@ursabot please benchmark

@ursabot
Copy link

ursabot commented Sep 6, 2022

Benchmark runs are scheduled for baseline = 7e7b8e1 and contender = 4575b26. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.58% ⬆️0.2%] test-mac-arm
[Failed ⬇️0.27% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.14% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 4575b269 ec2-t3-xlarge-us-east-2
[Failed] 4575b269 test-mac-arm
[Failed] 4575b269 ursa-i9-9960x
[Finished] 4575b269 ursa-thinkcentre-m75q
[Finished] 7e7b8e1f ec2-t3-xlarge-us-east-2
[Finished] 7e7b8e1f test-mac-arm
[Failed] 7e7b8e1f ursa-i9-9960x
[Finished] 7e7b8e1f ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@kevingurney
Copy link
Member Author

Thanks for re-running the benchmarks @raulcd! I was just about to re-run them myself, but had to step away.

@raulcd
Copy link
Member

raulcd commented Sep 6, 2022

hi @kevingurney I don't think the benchmark issues are related to this PR, I've seen this other PR reporting the same issues:
#14019 (comment)

zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
…pache#13975)

# Overview

This pull request:

1. Removes hard-coded dependencies on "master" as the default branch name in the crossbow infrastructure and CI template files.

# Implementation

1. Removed comment/text references to "master" branch, including URLs to other repositories.
2. Modified `core.py` to add a new `default_branch` property and a new method `is_default_branch`, for checking whether on the default branch, to the `Target` class.
3. Modified CI template files to use the new `is_default_branch` function to check whether on the default branch.

# Testing

1. Using [lafiona/crossbow](https://github.com/lafiona/crossbow) as a queue repository for qualification.
2. Ran modified template jobs. All failures appeared to be unrelated to the changes.
3. The branch names for all relevant qualification jobs are prefixed with `build-34-*`. 
4. Example of a passing job: [https://github.com/lafiona/crossbow/actions/runs/2920227769](https://github.com/lafiona/crossbow/actions/runs/2920227769)
5. Example of a failing job: [https://github.com/lafiona/crossbow/runs/7998190113](https://github.com/lafiona/crossbow/runs/7998190113) - in this example, the *"Push Docker Image"* workflow step is not included, since we are not on the default branch. The failure appears to be related to issues fetching R package resources and not related to the default branch checking logic. There were a variety of other kinds of failures, but none of them appear related to the default branch checking logic.

# Future Directions

1. Remove "master" from `default_branch` name property of `Target` class.
2. Remove all remaining uses of "master" terminology in crossbow.
3. [ARROW-17512](https://issues.apache.org/jira/browse/ARROW-17512): Address minor issues with crossbow documentation.

# Notes

1. Thank you to @lafiona for her help with this pull request!
2. Due to unexpected technical issues, we opened this pull request as a follow up to apache#13750. Please see apache#13750 for more discussion regarding qualification efforts.

Lead-authored-by: Kevin Gurney <kgurney@mathworks.com>
Co-authored-by: Fiona La <fionala7@gmail.com>
Signed-off-by: Alessandro Molina <amol@turbogears.org>
@kevingurney kevingurney deleted the ARROW-15693 branch August 21, 2023 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants