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

ci: split windows x86 job #2943

Merged
merged 1 commit into from
Apr 24, 2024
Merged

Conversation

kmantel
Copy link
Collaborator

@kmantel kmantel commented Apr 16, 2024

Windows x86 ci runs can fail because the container runs out of memory
during testing. This is caused by memory leaks in psyneulink and limited
available memory on the containers. As a workaround, running the full
set of tests over multiple jobs reduces peak memory usage.

Three jobs replace the original Windows x86 build, running pytest with:

  1. -m llvm
  2. -m "not llvm and composition"
  3. -m "not llvm and not composition"

Jobs with the same OS, python version, and architecture overwrite
uploaded dist packages.

Copy link

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

No differences!

...

See CI logs for the full diff.

@jvesely
Copy link
Collaborator

jvesely commented Apr 16, 2024

Does splitting by tag also address the issue?
e.g.

          - python-version: '3.8'
            python-architecture: 'x86'
            os: windows
            extra-args: '-m llvm'

          - python-version: '3.8'
            python-architecture: 'x86'
            os: windows
            extra-args: '-m not llvm'

(splitting by other tags should work too)

@kmantel
Copy link
Collaborator Author

kmantel commented Apr 17, 2024

It's a good suggestion - running multiple pytest commands in the job immediately fails the entire job if one command fails, so the entire suite may not be run.

Is there an additional benefit to splitting by llvm? not llvm still gives the memory error in that case, so I'd need to make three jobs.

extra-args: '-m llvm'
extra-args: '-m not llvm and composition'
extra-args: '-m not llvm and not composition'

but just splitting by composition seems to be ok also

Copy link

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

No differences!

...

See CI logs for the full diff.

@@ -181,10 +188,14 @@ jobs:
timeout-minutes: 180
run: pytest --junit-xml=tests_out.xml --verbosity=0 -n logical ${{ matrix.extra-args }}

- name: Set extra-args env for artifacts
run: echo "EXTRA_ARGS_DESC=$(echo ${{ matrix.extra-args }} | tr ' ="' '_\-\0')" >> $GITHUB_ENV
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a comment explaining the not allowed characters in artifact names?
Using >> $GITHUB_OUTPUT and steps.chosen-step-id.output below would be more targetted and avoid setting an env var for all follow up steps

Copy link
Collaborator Author

@kmantel kmantel Apr 19, 2024

Choose a reason for hiding this comment

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

I will. It looks like only double quote is actually disallowed, so I can remove the other substitutions. But I also think space->underscore is nice although unnecessary

@@ -209,7 +220,7 @@ jobs:

- name: Upload dist packages
uses: actions/upload-artifact@v4
if: matrix.version-restrict == ''
if: matrix.version-restrict == '' && (! contains(matrix.python-architecture, 'x86') || ! contains(matrix.extra-args, 'not composition'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

does:

overwrite: matrix.python-architecture == 'x86'

work?
the dist packages should be identical and it would be a bit easier to read

can you also add one sentence about the necessary adjustments to upload to the git commit description

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 looks like overwrite won't allow expressions, only true/false, unless I can't figure how to do it correctly. overwrite: true can still work and would currently only affect the x86. If we needed to add other jobs later without overwriting, we'd have to change the name anyway

Windows x86 ci runs can fail because the container runs out of memory
during testing. This is caused by memory leaks in psyneulink and limited
available memory on the containers. As a workaround, running the full
set of tests over multiple jobs reduces peak memory usage.

Three jobs replace the original Windows x86 build, running pytest with:
1. -m llvm
2. -m "not llvm and composition"
3. -m "not llvm and not composition"

Jobs with the same OS, python version, and architecture overwrite
uploaded dist packages.
@kmantel kmantel changed the title ci: split x86 pytests by directory ci: split windows x86 job Apr 23, 2024
Copy link

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

diff -r docs-base/Mechanism.html docs-head/Mechanism.html
379c379
< <strong>comparison_operation</strong> argument, that is used to set the LinearCombination function’s <a class="reference internal" href="CombinationFunctions.html#psyneulink.core.components.functions.combinationfunctions.CombineMeans.operation" title="psyneulink.core.components.functions.combinationfunctions.CombineMeans.operation"><code class="xref any py py-attr docutils literal notranslate"><span class="pre">operation</span></code></a> parameter.</p>
---
> <strong>comparison_operation</strong> argument, that is used to set the LinearCombination function’s <a class="reference internal" href="IntegratorFunctions.html#psyneulink.core.components.functions.stateful.integratorfunctions.DualAdaptiveIntegrator.operation" title="psyneulink.core.components.functions.stateful.integratorfunctions.DualAdaptiveIntegrator.operation"><code class="xref any py py-attr docutils literal notranslate"><span class="pre">operation</span></code></a> parameter.</p>
diff -r docs-base/RefactoredLearningGuide.html docs-head/RefactoredLearningGuide.html
282c282
< <p>Notice that we no longer have to extract the target node from the <code class="xref any docutils literal notranslate"><span class="pre">add_backpropagation_learning_pathway</span> <span class="pre">method</span></code>, and can instead pass the targets as output nodes mapped to values, in a new parameter called <code class="xref any docutils literal notranslate"><span class="pre">targets</span></code> in the <a class="reference internal" href="AutodiffComposition.html#psyneulink.library.compositions.autodiffcomposition.AutodiffComposition.learn" title="psyneulink.library.compositions.autodiffcomposition.AutodiffComposition.learn"><code class="xref any py py-meth docutils literal notranslate"><span class="pre">learn</span></code></a> method.</p>
---
> <p>Notice that we no longer have to extract the target node from the <code class="xref any docutils literal notranslate"><span class="pre">add_backpropagation_learning_pathway</span> <span class="pre">method</span></code>, and can instead pass the targets as output nodes mapped to values, in a new parameter called <code class="xref any docutils literal notranslate"><span class="pre">targets</span></code> in the <a class="reference internal" href="EMComposition.html#psyneulink.library.compositions.emcomposition.EMComposition.learn" title="psyneulink.library.compositions.emcomposition.EMComposition.learn"><code class="xref any py py-meth docutils literal notranslate"><span class="pre">learn</span></code></a> method.</p>
287c287
< its constructor to be runtime parameters of its <a class="reference internal" href="AutodiffComposition.html#psyneulink.library.compositions.autodiffcomposition.AutodiffComposition.learn" title="psyneulink.library.compositions.autodiffcomposition.AutodiffComposition.learn"><code class="xref any py py-meth docutils literal notranslate"><span class="pre">learn</span></code></a> method.</p>
---
> its constructor to be runtime parameters of its <a class="reference internal" href="EMComposition.html#psyneulink.library.compositions.emcomposition.EMComposition.learn" title="psyneulink.library.compositions.emcomposition.EMComposition.learn"><code class="xref any py py-meth docutils literal notranslate"><span class="pre">learn</span></code></a> method.</p>

...

See CI logs for the full diff.

@kmantel kmantel merged commit 427808e into PrincetonUniversity:devel Apr 24, 2024
34 checks passed
@kmantel kmantel deleted the tests-x86-split branch April 24, 2024 03:31
jvesely added a commit to jvesely/PsyNeuLink that referenced this pull request May 18, 2024
jvesely added a commit to jvesely/PsyNeuLink that referenced this pull request May 18, 2024
jvesely added a commit to jvesely/PsyNeuLink that referenced this pull request May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants