Skip to content

Conversation

@stevenhua0320
Copy link
Contributor

Closes #213 @sbillinge Ready to review

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

I found out from chatgpt hat this default is ignored if it is a workflow_call. This is why the matrix didn't fall back when inputs.python_version wasnt passed correctly in our workflow. Good thing too or we wouldn't have discovered the problem.

The fix is to put it directly in the python_version: field after ||.

But while we debug why that variable is not being passed correctly, we may not want any fallback, so let's just delete it for now

@stevenhua0320
Copy link
Contributor Author

@sbillinge I have made that commit here, unfortunately, there seems to be some subprocess of the workflow that enforce to this repo instead of my local fork. So, I cannot test it directly unless I figure out all subprocesses in the workflow and direct them to my forked repo.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

Is it possible to put in an echo of where the matrix is coming from for debuggong later? Something like "Python versions obtained dynamically: {{ inputs.python_versions }}" or something like that? Of it is empty, then trigger another message of what it is falling back to

@sbillinge
Copy link
Contributor

Thanks, this looks good. Were you able to figure out why the dynamic inputs.puthon_version is (presumably) not coming through to this sub-module? That is the next thing

@stevenhua0320
Copy link
Contributor Author

@sbillinge Well, I'm still stuck on in the subprocess the workflow would start to use the release-script from scikit-package/release-script instead of stevenhua0320/release-script on my local branch... Still figuring out and looking through where it triggers.

The error message confirms that it would still use this repository's branch to test on my local:

Error when evaluating 'strategy' for job 'build-wheel'. scikit-package/release-scripts/.github/workflows/_build-python-wheel-extension.yml@4b3aa9e24f0523dd96b3f9a2002bb575d9180867 (Line: 70, Col: 17): Unexpected value '3.13'

which is the same error for your test on release diffpy.pdffit2 in the morning as shown below.
Screenshot 2025-12-03 at 8 37 08 PM

@stevenhua0320
Copy link
Contributor Author

But apart from that, for the potential problem of not getting right version here, I'm suspecting it is because we have incompatible definition in python_version from the main process and subprocess. In diffpy.pdffit2, we use the workflow of _build-wheel-release-upload.yml first, where here we define python_version as this:

python_version:
        description: 'Python version to use. If not provided (or set to 0), will be detected from pyproject.toml'
        default: 0
        required: false
        type: number

And then in the job we still take python_version as a type of number:

build-wheel:
    needs: [get-python-version]
    uses: ./.github/workflows/_build-wheel.yml
    with:
      project: ${{ inputs.project }}
      c_extension: ${{ inputs.c_extension }}
      # Convert from number to string
      python_version: ${{ fromJSON(needs.get-python-version.outputs.python_version) }}

However, in the subprocess, after getting into _build-wheel.yml, it will get into another subprocess to _build-python-wheel-extension.yml, where we define python_version as this:

python_version:
        description: 'Python versions to use for building'
        required: true
        type: string

I'm not sure whether we need to keep the type of python_version from main process to subprocess the same, but it is safer to keep the two type the same.

@sbillinge
Copy link
Contributor

I think if you can make a testing rig where you write a workflow that calls your workflow in the same way that we do in our main one and then trigger it that way, could that work for testing maybe?

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.

fix: set fallback default versions to 3.12 -> 3.14 in ciwh workflow

2 participants