Skip to content

Conversation

AndyAyersMS
Copy link
Member

Since it needs to install packages.

Since it needs to install packages.
@ghost
Copy link

ghost commented Dec 7, 2023

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

Issue Details

Since it needs to install packages.

Author: AndyAyersMS
Assignees: AndyAyersMS
Labels:

area-Infrastructure-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

@BruceForstall PTAL
cc @dotnet/jit-contrib

Less certain how to do this for windows, since we're invoking the py.exe launcher instead of python.exe directly, and so the approach I use here of not activating the venv but instead using the full path to the venv seems a bit more fragile. Activation might work too, but it wasn't clear to me how durable the venv was.

Successful-ish run here: https://dev.azure.com/dnceng/internal/_build/results?buildId=2330788&view=logs&j=5eaadb19-0c1f-5ec8-5e1a-fa86334b456a

It still isn't completely green but I think the smoke test failures may be unrelated? @TIHan any ideas there?

@BruceForstall
Copy link
Contributor

Less certain how to do this for windows, since we're invoking the py.exe launcher instead of python.exe directly, and so the approach I use here of not activating the venv but instead using the full path to the venv seems a bit more fragile. Activation might work too, but it wasn't clear to me how durable the venv was.

Shouldn't it work the same way for Windows? If I try locally, I get a venv\Scripts\python.exe. Couldn't you define (for Windows):

      - name: PythonSetupScript
        value: 'py -3 -m venv $(Build.SourcesDirectory)\venv'
      - name: PythonScript
        value: '$(Build.SourcesDirectory)\venv\Scripts\python.exe'
      - name: PipScript
        value: '$(Build.SourcesDirectory)\venv\Scripts\python.exe -m pip'

(I also see a venv\Scripts\pip3.exe, but maybe the -m pip format is preferred?)

@AndyAyersMS
Copy link
Member Author

@BruceForstall added similar for windows

Validation run was a bit ragged (for other reasons) https://dev.azure.com/dnceng/internal/_build/results?buildId=2331419&view=results

@AndyAyersMS
Copy link
Member Author

Addresses the SPMI part of #49464

Should be easy enough to fix the rolling build too.

Copy link
Contributor

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

Please remove the "(on linux)" string.

(sadly, I wrote that comment a couple days ago but apparently never "published" it :-( )

- ${{ parameters.steps }}

- script: $(PythonSetupScript)
displayName: Enable python venv (on linux)
Copy link
Contributor

Choose a reason for hiding this comment

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

This runs on Mac as well

Suggested change
displayName: Enable python venv (on linux)
displayName: Enable python venv

@BruceForstall
Copy link
Contributor

Should be easy enough to fix the rolling build too.

Do I hear a volunteer?

It's much faster turnaround to test the rolling build.

@AndyAyersMS
Copy link
Member Author

Should be easy enough to fix the rolling build too.

Do I hear a volunteer?

It's much faster turnaround to test the rolling build.

Ok, done.

https://dev.azure.com/dnceng/internal/_build/results?buildId=2332020&view=logs&j=b4253421-1bc6-52ba-006c-9cc722a95c07&t=6f3d3f29-d67e-5108-fbe5-476b460944bd

There are still more yml files with the old-style python/pip lurking in them, maybe they run only on windows and so don't (yet) cause problems?

@BruceForstall BruceForstall changed the title Use python venv for SPMI collection on linux Use python venv for SPMI collection / jit rolling build Dec 8, 2023
@BruceForstall
Copy link
Contributor

The changes look good to me.

There are still more yml files with the old-style python/pip lurking in them, maybe they run only on windows and so don't (yet) cause problems?

AFAIK, jitrollingbuild and SPMI collect are the only JIT-owned YML that use pip to upgrade Python packages.

@AndyAyersMS
Copy link
Member Author

The changes look good to me.

There are still more yml files with the old-style python/pip lurking in them, maybe they run only on windows and so don't (yet) cause problems?

AFAIK, jitrollingbuild and SPMI collect are the only JIT-owned YML that use pip to upgrade Python packages.

There is one more in eng/pipelines/common/templates/runtimes/run-test-job.yml

@BruceForstall
Copy link
Contributor

There is one more in eng/pipelines/common/templates/runtimes/run-test-job.yml

Ah, right, the coreclr "run" test job does all the spmi processing itself.

So that points out some other places that should change:

  1. libraries "run" collections are similar, but PipScript and PythonScript definitions are spread out a little: eng\pipelines\libraries\superpmi-postprocess-step.yml, eng\pipelines\libraries\superpmi-collect-variables.yml, eng\pipelines\libraries\run-test-job.yml (this doesn't need to change)
  2. Several places define PipScript but don't use it. Maybe we should just remove it if we're not going to create a venv, to reduce confusion: eng\pipelines\coreclr\templates\run-superpmi-replay-job.yml, eng\pipelines\coreclr\templates\run-superpmi-diffs-job.yml, eng\pipelines\coreclr\templates\run-superpmi-asmdiffs-checked-release-job.yml, eng\pipelines\coreclr\templates\jit-run-exploratory-job.yml

@AndyAyersMS
Copy link
Member Author

None of the rest currently cause problems, so I'm going to merge this bit when CI is done so we get clean(er) collections over the weekend and come back for the rest later.

@AndyAyersMS AndyAyersMS merged commit c9998da into dotnet:main Dec 9, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants