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: fix failing py3.13 on osx-64 #258

Merged
merged 3 commits into from
Jan 20, 2025
Merged

ci: fix failing py3.13 on osx-64 #258

merged 3 commits into from
Jan 20, 2025

Conversation

Tieqiong
Copy link
Contributor

@sbillinge please check, thanks

wxpython 4.2.2 support py3.13 for osx-64, but it can only be installed with OSX version >=11.0 (Big Sur). Seems like set-up miniconda detect OSX version of GitHub runner to be 10.16, here we mock it to be 11.0 in order to resolve wxpython.

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.10%. Comparing base (c446969) to head (98bb0b0).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #258   +/-   ##
=======================================
  Coverage   95.10%   95.10%           
=======================================
  Files          23       23           
  Lines        1123     1123           
=======================================
  Hits         1068     1068           
  Misses         55       55           

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.

Here we maybe need to make an issue to remove this mock, or at least comment it out in case we need to redo this fix in the future) when the GitHub azure runner is updated to 11.0 or greater?

Also, will this affect some of our users? Do we need to do some vesrsion pinning or put something into the docs or into diffpy-users in the future?

@Tieqiong
Copy link
Contributor Author

@sbillinge The root problem is not with Github runners. There are also many other work arounds for this problem (and possible better solution than mocking the macOS version). Let me first explain it then we can choose the best way.

So everything starts with wxpython, which set its 4.2.2 osx-64 py3.13 build work only with __osx>=11.0. This is not a tough constrain, as almost all Mac products after 2012 are able to update to macOS 11.0. The problem is the way conda gets this macOS version info depends on how the python in its base environment got compiled. Specifically it can either be >=10.16,<11.0 if CPython was built with macOS SDK <11 or >=11.0 if built with SDK >=11. This is a result of the 10.x compatibility rules that comes into play when reporting the OS version starting with Big Sur.

From Conda's latest doc they mentioned "... we usually install the 10.10 SDK..." which means if an user (using Mac with osx-64) for example

  1. always use the default channel, so their base python is compiled with SDK<11.0 so that "__osx<11.0", or
  2. managed to use conda-forge but never update anything in their base from their initial conda installation, or
  3. like our GitHub runner, running the setup-miniconda action from fresh so their base python is from default channel

so that it's possible their "__osx" is not their actual macOS version and is <11.0, which means they can't install wxpython in py3.13.


So why this mocking works here? it's because the actual macOS version is 13.7.2 for macos-13 (from GitHub/runner-image), but the base python can only recognize it as 10.16, which is <11.0. I manually changed it to 11.0, which will not effectively breaks anything (as 11.0<13.7.2).

What could be another possible better way? I noticed for the setup-miniconda action we are using now they also provide an option to use miniforge, which is almost the same as miniconda but everything are from conda-forge. This can avoid installing default python in base in the first place.

Some other ways could be manually updating base, which I think will be much more complex and is a bad idea.


So will this affect some of our users?

I would say very unlikely, but it could happen if the user is 1. using an Intel Mac (so osx-64) and 2. new to conda. I wouldn't concern much about it, but it would be nice to put it somewhere just in case. In fact I'd expect similar issue become more and more when everything gets versioned up, and this is neither our packages creating this problem nor specific to us. Everything using wxpython 4.2.2 and possible other packages in the future will encounter this.

@sbillinge
Copy link
Contributor

@Tieqiong Mostly I am concerned with minimizing technical debt moving forward. Every time we do a bespoke edit for some weird edge-case that is relevant at some moment in time it is more junk in our code that will turn around and bite us later because we forget about it, naturally (don't want to clutter my brain with all these things), and some weird bug turns up later that takes us days to get to the bottom of. So, in general, the solution that minimizes but still gets the job done, is the one to be preferred.

For example, if this is a user bug (not a CI testing one) and it crops up because of a not properly synchronized conda environment, my preferred fix would be to find a way to tell the users to update their conda environments. That fix would mean we make zero edits to our functional code (we may have to put in some warning messages).

If there is a GH runner issue, could we add a line to the workflow that runs conda update python in base, something like that?

@Tieqiong
Copy link
Contributor Author

@sbillinge I'd go with directly using miniforge, which never use the default channel and thus no base default python problem. It do the job and is simple. Will make a commit and we can discuss

@sbillinge
Copy link
Contributor

@sbillinge I'd go with directly using miniforge, which never use the default channel and thus no base default python problem. It do the job and is simple. Will make a commit and we can discuss

Yay, yes, this is the best!

@Tieqiong
Copy link
Contributor Author

@sbillinge Shall we merge this? Once the CI passes, we can proceed with an RC release and encourage people to start testing the app!

@sbillinge
Copy link
Contributor

sorry @Tieqiong I had made a review but obviously forgot to submit it.... Basically I wanted a better news to describe this, but I just went ahead and made it myself to move this along.

@sbillinge sbillinge merged commit 792e22d into diffpy:main Jan 20, 2025
3 checks passed
@sbillinge
Copy link
Contributor

@sbillinge Shall we merge this? Once the CI passes, we can proceed with an RC release and encourage people to start testing the app!

@Tieqiong @bobleesj OK, I merged and it passed CI. We still have a few issues to close though:
https://github.com/diffpy/diffpy.pdfgui/milestone/3

I am working on #167 so no need to do that. I will promise to complete it today.

Some of those issues are old and just need to be checked. If they are from an outside user, let's do our best to think about whether we think it is fixed or not by our recent updates. If we think it is, let's move the issue to the full-release milestone with a message to the OP to test the new pre-release and let us know whether it is fixed or not. When we hear back that it is fixed we can close.

In this way we can work to close or move all the issues on the milestone.

There is one feature request (replace the Nyquist-Shannon interpolator with the ws one. I think this is more or less a two-liner in the code...which interpolator we import from diffpy.utils and which we call, though it may need tests updating. In any case, let's try and do it for the rc release, but also let's try and get this out the door by the end of today.

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