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

Update SuperPMI CI automation #60375

Merged

Conversation

BruceForstall
Copy link
Member

@BruceForstall BruceForstall commented Oct 13, 2021

A number of changes:

  1. Move the SuperPMI replay pipeline to the public instance, so it can be
    triggered by GitHub PRs.
  2. Rename SuperPMI collection scripts to contain "collect" in their names,
    to distinguish them from the "replay" scripts.
  3. Remove a lot of unused copy/paste cruft
  4. Create a new azdo_pipelines_util.py script for all the CI scripts to depend
    on, so they don't import the superpmi.py script, or each other.
  5. Some changes to simplify the Python imports and be more consistent in imported
    API usage.

Fixes: #59559

A number of changes:
1. Move the SuperPMI replay pipeline to the public instance, so it can be
triggered by GitHub PRs.
2. Rename SuperPMI collection scripts to contain "collect" in their names,
to distinguish them from the "replay" scripts.
3. Remove a lot of unused copy/paste cruft
4. Create a new azdo_pipelines_util.py script for all the CI scripts to depend
on, so they don't import the superpmi.py script, or each other.
5. Some changes to simplify the Python imports and be more consistent in imported
API usage.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 13, 2021
@ghost
Copy link

ghost commented Oct 13, 2021

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

A number of changes:

  1. Move the SuperPMI replay pipeline to the public instance, so it can be
    triggered by GitHub PRs.
  2. Rename SuperPMI collection scripts to contain "collect" in their names,
    to distinguish them from the "replay" scripts.
  3. Remove a lot of unused copy/paste cruft
  4. Create a new azdo_pipelines_util.py script for all the CI scripts to depend
    on, so they don't import the superpmi.py script, or each other.
  5. Some changes to simplify the Python imports and be more consistent in imported
    API usage.
Author: BruceForstall
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@BruceForstall
Copy link
Member Author

@kunalspathak This change looks good to me. Can you check the Antigen results to tell me if anything looks wrong? I'm not sure if it's supposed to be clean, or if there are new failures here due to my change.

https://dev.azure.com/dnceng/public/_build/results?buildId=1419755&view=results

@BruceForstall
Copy link
Member Author

I pushed a change to not overwrite the superpmi collection. I'd like to:

  1. merge this
  2. test superpmi collect pipeline, and iterate on the internal pipeline if necessary
  3. remove the change to not overwrite the existing superpmi collection

@kunalspathak
Copy link
Member

kunalspathak commented Oct 14, 2021

Wonder why it doesn't show superpmi-replay runs in the checks - https://dev.azure.com/dnceng/public/_build/results?buildId=1419680&view=results

@BruceForstall - I think one of the only thing remaining from #59559 is having -jit at the end of yml file.

@@ -30,10 +30,19 @@
<SuperpmiLogsLocation>%HELIX_WORKITEM_UPLOAD_ROOT%</SuperpmiLogsLocation>
<!-- Workaround until https://github.com/dotnet/arcade/pull/6179 is not available -->
<HelixResultsDestinationDir>$(BUILD_SOURCESDIRECTORY)\artifacts\helixresults</HelixResultsDestinationDir>
<WorkItemCommand>$(Python) $(ProductDirectory)\superpmi-replay.py -jit_directory $(ProductDirectory)</WorkItemCommand>
<WorkItemCommand>$(Python) $(ProductDirectory)\superpmi_replay.py -jit_directory $(ProductDirectory)</WorkItemCommand>
<WorkItemTimeout>5:00</WorkItemTimeout>
Copy link
Member

Choose a reason for hiding this comment

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

I am actually wondering if we should reduce this to 2:00 or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, 5:00 might be too long.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like average times for the runs are 2:15 to 2:30, with one hitting 2:52. So maybe 3:15 would be reasonable.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

print(define_variable_format.format(name, value)) # set variable


class TempDir:
Copy link
Member

Choose a reason for hiding this comment

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

There is also a copy of this class in jitrollingbuild.py , can we delete that as well?

Copy link
Member

Choose a reason for hiding this comment

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

Did you also check if there is any reusable functionality in superpmi.py that should be moved to this file or some functions from this file that can be used in superpmi.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can update jitrollingbuild.py/superpmi.py later, if needed.

Actually, I specifically did not want to have superpmi.py depend on a "...pipelines....py" file, so I decided to duplicate the code. We could add a more general "util.py" file. However, I was a little torn because there already is a "utilities.py" file for the "other" set of py scripts, and I don't really want to mix with those.

@BruceForstall BruceForstall merged commit feed66d into dotnet:main Oct 14, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 14, 2021
@BruceForstall BruceForstall deleted the RenameSuperpmiCollectionPipeline branch December 28, 2022 01:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve superpmi scripts
2 participants