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

Temporary fix for version dependency issues #137

Merged
merged 55 commits into from
Nov 8, 2024

Conversation

ljlamarche
Copy link
Collaborator

@ljlamarche ljlamarche commented Nov 7, 2024

Description

This PR pins the version of numpy and python used in pyproject.toml and clarifies in the documentation that apexpy is currently not compatible with numpy 2.0 and python 3.12. A new minor release should be published to help users avoid some confusion with installation issues.

This is a temporary fix for #134 and #135. These issues should not be closed as more comprehensive fixes should be implemented in the future.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Release candidate

How Has This Been Tested?

Installation has been tested in a clean Python 3.11 environment to ensure expected behavior (successful installation of apexpy with numpy 1.26). Installation in a python 3.12 behavior fails, but it is now documented that apexpy does not support python 3.12.

From apexpy root directory:

pip install .

Test Configuration

  • Operating system: MacOS
  • Python version number: 3.11/3.12
  • Compiler with version number: gfortran 13.1.0

Checklist

  • Make sure you are merging into the develop (not main) branch
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • Add a note to Changelog.rst, summarising the changes
  • Add yourself to AUTHORS.rst and .zenodo.json

@aburrell aburrell changed the base branch from main to develop November 7, 2024 15:05
Copy link
Owner

@aburrell aburrell left a comment

Choose a reason for hiding this comment

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

I think we also need to update the unit test python versions.

Comment on lines 25 to 67
#@pytest.fixture()
#def igrf_file(max_attempts=100):
# """A fixture for handling the coefficient file.
#
# Parameters
# ----------
# max_attempts : int
# Maximum rename attemps, needed for Windows (default=100)
#
# """
# # Ensure the coefficient file exists
# original_file = os.path.join(os.path.dirname(apexpy.helpers.__file__),
# 'igrf13coeffs.txt')
# tmp_file = "temp_coeff.txt"
# assert os.path.isfile(original_file)
#
# # Move the coefficient file
# for _ in range(max_attempts):
# try:
# shutil.move(original_file, tmp_file)
# break
# except Exception:
# pass
# yield original_file
#
# # Move the coefficient file back
# for _ in range(max_attempts):
# try:
# shutil.move(tmp_file, original_file)
# break
# except Exception:
# pass
# return


#def test_set_epoch_file_error(igrf_file):
# """Test raises OSError when IGRF coefficient file is missing."""
# # Test missing coefficient file failure
# with pytest.raises(OSError) as oerr:
# apexpy.Apex()
# error_string = "File {:} does not exist".format(igrf_file)
# assert str(oerr.value).startswith(error_string)
# return
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
#@pytest.fixture()
#def igrf_file(max_attempts=100):
# """A fixture for handling the coefficient file.
#
# Parameters
# ----------
# max_attempts : int
# Maximum rename attemps, needed for Windows (default=100)
#
# """
# # Ensure the coefficient file exists
# original_file = os.path.join(os.path.dirname(apexpy.helpers.__file__),
# 'igrf13coeffs.txt')
# tmp_file = "temp_coeff.txt"
# assert os.path.isfile(original_file)
#
# # Move the coefficient file
# for _ in range(max_attempts):
# try:
# shutil.move(original_file, tmp_file)
# break
# except Exception:
# pass
# yield original_file
#
# # Move the coefficient file back
# for _ in range(max_attempts):
# try:
# shutil.move(tmp_file, original_file)
# break
# except Exception:
# pass
# return
#def test_set_epoch_file_error(igrf_file):
# """Test raises OSError when IGRF coefficient file is missing."""
# # Test missing coefficient file failure
# with pytest.raises(OSError) as oerr:
# apexpy.Apex()
# error_string = "File {:} does not exist".format(igrf_file)
# assert str(oerr.value).startswith(error_string)
# return

pyproject.toml Outdated Show resolved Hide resolved
@ljlamarche
Copy link
Collaborator Author

Re updating unit tests, agreed. That would be adding 3.11 and 3.12 to this line, correct?

https://github.com/ljlamarche/apexpy/blob/aaf50742fd4d4ad38c16205cbfdfbc85260b5565/.github/workflows/main.yml#L20

We'd expect 3.12 to fail at this point.

Updated the python versions tested on GitHub Actions.
Added a yaml file for ReadTheDocs.
Updated the supported python versions in the documentation.
Moved the minimum supported python version to 3.9.
@aburrell
Copy link
Owner

aburrell commented Nov 7, 2024

I just added up to 3.11, since we aren't supporting 3.12. We can add that in when we expect it to succeed.

@ljlamarche
Copy link
Collaborator Author

Sounds good. Just so you know, when you install in python 3.12, it still gives the error listed in #135 not a convenient "not supported" message unfortunately. It looks like pip tries to build the package before checking if it's a supported version, and I couldn't figure out if there was a way to make it act any differently.

aburrell and others added 18 commits November 7, 2024 13:28
Try and avoid installing apexpy on RTD.
Updated the change log.
A new fix for just installing the desired packages.
Cycled supported python versions in the setup.cfg file.
Added flake8 to the test requirements.
Cycled the versions of github actions used, implementing the coveralls action.
Fixed a typo in the action name.
Removed extra whitespace.
Removed gcc version specification from the mac installation call.
Fixed the pip installation call in the tests.
Fixed test bugs:
- changed pytest flags and
- fixed pip install call for numpy.
MacOS is not supporting python 3.9 and 3.10.  Changed OS for these versions to one that supports them.  Also specifically call out gcc-14.
Put the OS names in quotation marks.
Comment line seemed to cause an error.
There's something wrong with the job matrix, attempt a fix.
Added space after dash.  Also tried containsValue function for new macOS runs.
@ljlamarche
Copy link
Collaborator Author

I think I fixed the unit tests so they pass, but now it looks like there's an error with reporting the tests? Will try to look more tomorrow...

Determine where the xml file is located.
Removed tab from yaml.
Trying a new debug line.
Remove debug and move lines, hope for the best.
Try to use directory one level up in coveralls app.
Remove tab from yaml.
Added a debug line and also attempted to specify the output filename through pytest-cov.
Still leaving the tabs in.
Changed the filename to exclude directory.
Added more debug lines.
Removed tabs.
Change the debug command to include hidden files.
Try removing rootdir from pytest command.
Remove tab.
Trying to simplify the call again.
@aburrell
Copy link
Owner

aburrell commented Nov 8, 2024

Ok, the current problem is that pytest-cov isn't creating an output file. I don't know why, the commands work locally on another package for me, but also fail locally (running in test directory). I am not sure how to debug this.

Try coverage as a way to create the necessary output.
Fully revert to coverage, these commands work locally and the pytest ones don't.
Remove tab from yaml.
@aburrell
Copy link
Owner

aburrell commented Nov 8, 2024

Ok, I had one more idea. The new command work locally, but not here 😢

@aburrell
Copy link
Owner

aburrell commented Nov 8, 2024

Alternative is to remove coveralls for now.

@ljlamarche
Copy link
Collaborator Author

I don't want to rehash things you've already tried, but are we using the Coveralls GitHub Action? The most common reason I've found for GH Actions to mysteriously stop working is version updates in these that you need to include in the workflow file.

@aburrell
Copy link
Owner

aburrell commented Nov 8, 2024

@ljlamarche yes, it's basically required to use now, so we're using it. The main problem is that that tests aren't generating a coverage file (in any format) after they run.

@ljlamarche
Copy link
Collaborator Author

Ok, I have no other brilliant ideas... Go ahead and disable for now if you're comfortable with that.

Try to remove the RC file from the coverage call.
This worked, remove the call for Windows as well.
@aburrell
Copy link
Owner

aburrell commented Nov 8, 2024

@ljlamarche we did it!

@aburrell aburrell merged commit dff9052 into aburrell:develop Nov 8, 2024
24 checks passed
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