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.

Sign up for GitHub

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

Jump to bottom

ci: update on-tag pypi release action #3078

Merged
merged 3 commits into from
Nov 5, 2024
Merged

ci: update on-tag pypi release action #3078

merged 3 commits into from
Nov 5, 2024

Conversation

kmantel
Copy link
Collaborator

@kmantel kmantel commented Oct 23, 2024

No description provided.

@github-actions GitHub Actions
Copy link

This PR causes the following changes to the html docs (ubuntu-latest-3.11):

diff -r docs-base/ModulatoryProjection.html docs-head/ModulatoryProjection.html
272c272
< <a class="reference internal" href="Port.html"><span class="doc">Port</span></a> are listed in the Port’s <a class="reference internal" href="Mechanism.html#id7" title="psyneulink.core.components.mechanisms.mechanism.Mechanism_Base.mod_afferents"><code class="xref any py py-attr docutils literal notranslate"><span class="pre">mod_afferents</span></code></a> attribute.</p>
---
> <a class="reference internal" href="Port.html"><span class="doc">Port</span></a> are listed in the Port’s <a class="reference internal" href="ParameterPort.html#psyneulink.core.components.ports.parameterport.ParameterPort.mod_afferents" title="psyneulink.core.components.ports.parameterport.ParameterPort.mod_afferents"><code class="xref any py py-attr docutils literal notranslate"><span class="pre">mod_afferents</span></code></a> attribute.</p>
diff -r docs-base/Port.html docs-head/Port.html
616c616
< <code class="xref any docutils literal notranslate"><span class="pre">PathWayProjections</span></code> (listed in its <a class="reference internal" href="Mechanism.html#id6" title="psyneulink.core.components.mechanisms.mechanism.Mechanism_Base.path_afferents"><code class="xref any py py-attr docutils literal notranslate"><span class="pre">path_afferents</span></code></a> attribute) as the variable for its
---
> <code class="xref any docutils literal notranslate"><span class="pre">PathWayProjections</span></code> (listed in its <a class="reference internal" href="#psyneulink.core.components.ports.port.Port_Base.path_afferents" title="psyneulink.core.components.ports.port.Port_Base.path_afferents"><code class="xref any py py-attr docutils literal notranslate"><span class="pre">path_afferents</span></code></a> attribute) as the variable for its
diff -r docs-base/TransferFunctions.html docs-head/TransferFunctions.html
2361c2361
< <p><a class="reference internal" href="#psyneulink.core.components.functions.transferfunctions.Exponential.bounds" title="psyneulink.core.components.functions.transferfunctions.Exponential.bounds"><code class="xref any py py-attr docutils literal notranslate"><span class="pre">bounds</span></code></a> – specifies the lower and upper limits of the result;  if there are none, the attribute is set to
---
> <p><a class="reference internal" href="OptimizationFunctions.html#psyneulink.core.components.functions.optimizationfunctions.GradientOptimization.bounds" title="psyneulink.core.components.functions.optimizationfunctions.GradientOptimization.bounds"><code class="xref any py py-attr docutils literal notranslate"><span class="pre">bounds</span></code></a> – specifies the lower and upper limits of the result;  if there are none, the attribute is set to

...

See CI logs for the full diff.

@kmantel kmantel mentioned this pull request Oct 23, 2024
Open
@github-actions GitHub Actions
Copy link

This PR causes the following changes to the html docs (ubuntu-latest-3.11):

No differences!

...

See CI logs for the full diff.

@kmantel kmantel marked this pull request as ready for review October 25, 2024 04:21
@kmantel kmantel requested a review from jvesely October 25, 2024 04:50
@jvesely
Copy link
Collaborator

jvesely commented Oct 25, 2024

The main problem with the workflow is that the GA security model doesn't allow jobs to depend on other jobs that include secrets. So the 'test-release' part cannot depend on 'create-python-dist', as long as the latter uses secrets to upload the packages. Either the upload needs to split off to its own job, or we need to find a way to work around the restriction.
There also isn't a good way to install pnl wheel on a 32bit Python. The pnl-test CI jobs hack around the torch requirement by removing it (and mdf) from the requirements file, but this is not possible for wheels/packages. we'd need to add something like a notorch extra if we want wheels/sdist working on 32-bit Python

The idea behind this test workflow was to check if the sdist/wheel packages were created correctly (to catch issues like #2040). I don't think we need the full copy of the special jobs from the main test files. Since we release universal wheel, we should be good enough to just check that both packages install OK on all supported platofrms

Not sure if there's a quick fix for the existing release scripts. I used to test them on a local repo creating test tags (and skipping uploads), because of GA security constraints.

It might be worth using this opportunity to adjust how CI should work combined with release scripts.
The regular CI jobs can install from wheels instead of editable local folder as is done currently. The wheels are already created in every pnl-test CI job so the steps only need to be shuffled around so the wheels package is used during testing.
The release script could then just test that installation via both sdist and wheel works and they both install the same files. It can just wait for the regular CI job on master instead of re-running the tests.

@kmantel
Copy link
Collaborator Author

kmantel commented Oct 26, 2024

The logs from the previous run have expired, but I do see "Skip output 'sdist' since it may contain secret.". On my fork, it doesn't seem to have a problem with this https://github.com/kmantel/PsyNeuLink/actions/runs/11527544088. Is that what you meant by how you tested it? Where it's possibly different repo or secrets settings that are letting it through. Or maybe the security model has changed since this was last looked at?

I can see the benefit of reworking CI to use wheels; if shifting to that, I think I'd just go ahead with the next release first without any CI changes.

@jvesely
Copy link
Collaborator

jvesely commented Oct 26, 2024

The logs from the previous run have expired, but I do see "Skip output 'sdist' since it may contain secret.". On my fork, it doesn't seem to have a problem with this https://github.com/kmantel/PsyNeuLink/actions/runs/11527544088. Is that what you meant by how you tested it? Where it's possibly different repo or secrets settings that are letting it through. Or maybe the security model has changed since this was last looked at?

I can see the benefit of reworking CI to use wheels; if shifting to that, I think I'd just go ahead with the next release first without any CI changes.

Thanks. hm, it looks like the rules now allow the existing flow. I think it makes sense to fix/resurrect the script independently of changes to the main CI jobs. I assumed that the test-release job will fail, but if it runs, it should do something useful.
Without syncing the special jobs in the matrix, LGTM.

jvesely
jvesely reviewed Oct 26, 2024
View reviewed changes
.github/workflows/test-release.yml
@@ -69,9 +69,60 @@ jobs:
strategy:
fail-fast: false
matrix:
python-version: [3.7, 3.8, 3.9]
os: [ubuntu-latest, macos-latest, windows-latest]
dist: [wheel, sdist]
Copy link
Collaborator

Choose a reason for hiding this comment

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

'dist' might still be needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's in the previous matrix that I'm putting back, and it's used by the - name: Install wheel and - name: Install sdist steps

.github/workflows/test-release.yml Outdated
python-architecture: ['x64']
extra-args: ['']
os: [ubuntu, macos, windows]
version-restrict: ['']
Copy link
Collaborator

Choose a reason for hiding this comment

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

'version-restrict' is not used in this job

.github/workflows/test-release.yml Outdated
dist: [wheel, sdist]
python-version: ['3.7', '3.11', '3.12']
python-architecture: ['x64']
extra-args: ['']
Copy link
Collaborator

Choose a reason for hiding this comment

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

'extra-args' are not used in this job

.github/workflows/test-release.yml Outdated
os: [ubuntu-latest, macos-latest, windows-latest]
dist: [wheel, sdist]
python-version: ['3.7', '3.11', '3.12']
python-architecture: ['x64']
Copy link
Collaborator

Choose a reason for hiding this comment

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

'python-architecture' is not used in the setup-python action, so it can be dropped from the matrix. It might be a good idea to test macos both aarch64 and x64, but if there's only one arch using the default is IMO sufficient.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the ci in general or for this workflow/wheel-based ci?

.github/workflows/test-release.yml Outdated
os: [ubuntu, macos, windows]
version-restrict: ['']
include:
# code-coverage build on macos python 3.9
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can skip the specialized jobs in release testing.
32-bit and version restrict tests need modifications of the requirements files which is not done in this workflow.

@kmantel kmantel force-pushed the ci-release branch 3 times, most recently from dd6248b to f960879 Compare October 31, 2024 05:10
@github-actions GitHub Actions
Copy link

This PR causes the following changes to the html docs (ubuntu-latest-3.11):

No differences!

...

See CI logs for the full diff.

1 similar comment
@github-actions GitHub Actions
Copy link

This PR causes the following changes to the html docs (ubuntu-latest-3.11):

No differences!

...

See CI logs for the full diff.

@kmantel kmantel force-pushed the ci-release branch 2 times, most recently from dd7a427 to 44368fc Compare November 5, 2024 03:00
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 5, 2024

This PR causes the following changes to the html docs (ubuntu-latest-3.11):

No differences!

...

See CI logs for the full diff.

@kmantel kmantel merged commit addc1be into PrincetonUniversity:devel Nov 5, 2024
36 checks passed
@kmantel kmantel deleted the ci-release branch November 5, 2024 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvesely jvesely jvesely left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kmantel @jvesely