Skip to content

Conversation

@SMoraisAnsys
Copy link
Contributor

@SMoraisAnsys SMoraisAnsys commented Sep 6, 2025

Changes to address #985.

@SMoraisAnsys SMoraisAnsys marked this pull request as ready for review September 7, 2025 16:25
@SMoraisAnsys SMoraisAnsys requested a review from a team as a code owner September 7, 2025 16:25
@MaxJPRey
Copy link
Contributor

MaxJPRey commented Sep 7, 2025

Thanks for taking care of that @SMoraisAnsys

@SMoraisAnsys SMoraisAnsys marked this pull request as draft September 7, 2025 19:04
Copy link
Member

@jorgepiloto jorgepiloto left a comment

Choose a reason for hiding this comment

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

Left some questions. The only thing I am concerned is that this causes some issues when using the actions since we are now imposing a specific version for some packages.

@SMoraisAnsys
Copy link
Contributor Author

SMoraisAnsys commented Sep 8, 2025

Left some questions. The only thing I am concerned is that this causes some issues when using the actions since we are now imposing a specific version for some packages.

I'm not sure we'll end up with any issue tbh. AFAIK the packages are only pinned for actions were we already had a working venv which was created from scratch and should have been modified after package installation.
Edit: Just found that at least the pytest action can be an issue when one wants to use it with randomize. In that case pytest-randomly is being installed before running the tests. Note that I also discovered a bug on the fly as the venv isn't activated before installing the package. Since this action is compatible with uv, pip and poetry, I'm not sure how we should handle the addition of pytest-randomly...

Guess it's a good candidate to leverage the pre release feature ? :D

@SMoraisAnsys
Copy link
Contributor Author

I've updated the code of the pytest action as follow

    - uses: ansys/actions/_logging@main
      if: inputs.randomize == 'true'
      with:
        level: "WARNING"
        message: >
          Installing pytest-randomly on top of the test dependencies as requested.
          This might lead to dependency conflicts. Consider adding pytest-randomly
          to your test dependencies instead.

    - name: "Randomize the order of the tests"
      shell: bash
      if: inputs.randomize == 'true'
      env:
        ACTION_PATH: ${{ github.action_path }}
      run: |
        if [[ "$BUILD_BACKEND" == 'poetry' ]]; then
          poetry run pip install pytest-randomly==3.16.0
        if [[ "$BUILD_BACKEND" == 'uv' ]]; then
          uv pip install pytest-randomly==3.16.0
        else
          python -m pip install pytest-randomly==3.16.0
        fi

@SMoraisAnsys SMoraisAnsys marked this pull request as ready for review September 8, 2025 15:46
@jorgepiloto jorgepiloto requested a review from moe-ad September 10, 2025 12:53
@SMoraisAnsys
Copy link
Contributor Author

@jorgepiloto @moe-ad It's been more than a month that this PR is open, should it be closed ? If not I can refresh my mind on its content cause I've lost track of the associated changes.

Copy link
Member

@jorgepiloto jorgepiloto left a comment

Choose a reason for hiding this comment

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

Last thing before merging this is to update the dependabot.yml so that it is aware about these new requirements.txt files.

@SMoraisAnsys
Copy link
Contributor Author

Last thing before merging this is to update the dependabot.yml so that it is aware about these new requirements.txt files.

Let's discuss about those changes during tomorrow's meeting to see if we are missing something.

@SMoraisAnsys SMoraisAnsys marked this pull request as draft October 28, 2025 14:48
@SMoraisAnsys
Copy link
Contributor Author

Following today's discussion, I'm switching to draft mode. Here are the available options to be explored (by order of personal taste):

  1. Use uv lock file (might require a TOML file to be around but should be dependabot comaptible and allow transient updates)
  2. Use poetry lock file - same as uv BUT uv is the default manager to install packages in our actions.
  3. Use pip freeze but this would require an additional job to handle the addition of transient dependency when dependabot opens a PR (say that dependabot bump A whose new version requires B then we would need to catch this update which wouldn't be caught by dependabot alone).

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.

6 participants