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.

Sign up for GitHub

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

Jump to bottom

Transition to find_package(Python3) (#610) #616

Merged
merged 19 commits into from
Oct 9, 2024
Merged

Transition to find_package(Python3) (#610) #616

merged 19 commits into from
Oct 9, 2024

Conversation

bartlettroscoe
Copy link
Member

Description

Addresses #610

Notes to reviewers

  • Prefer to review this PR commit-by-commit
  • There are some commits at the beginning that are not directly related to this change but were bundled with this PR to avoid extra overhead
  • Please make sure and review the updated documentation and CHANGELOG.md files

@bartlettroscoe
Fix underline length to match text
693a866 
Not sure why this was not caught earlier (but it was just a warning, not an
error).
@bartlettroscoe
Remove unneeded 'import imp' (TriBITSPub#610)
50c9c99 
On RHEL8 with Python 3.12, got the error message:

  No module named 'imp'
@bartlettroscoe
Move string-replace.py and token-replace.py into tribits/refactoring/ (…
9ffce30 
…TriBITSPub#610)

These are reusable helper tools that we might as well snapshot with the rest
of TriBITS.  (These don't have automated tests but it is easy to verify they
are working as they should.)
@bartlettroscoe
Fix help msg on -o argument (TriBITSPub#610)
33432fd 
Noticed this while working on TriBITSPub#610
@bartlettroscoe
Replace assert_() with assertIn() (TriBITSPub#610)
c0812eb 
With Python 3.12, the unittest.assert_() function no longer exists.
@bartlettroscoe
Assume standard Python3/pip3 install of docutils (TriBITSPub#610)
31bfe43 
No sense generating a message about not being able to find 'rst2html' when
modern systems don't even have that program.  It is called 'rst2htm.py' (and
'rst2latex.py').

This was tested with Python 3.12.

Signed-off-by: Roscoe A. Bartlett <[email protected]>
@bartlettroscoe
Don't set PYTHON_EXECUTABLE to python3 (TriBITSPub#610)
552fa62 
We want to use default behavior to just pick up python3 by default
@bartlettroscoe
Change to find_package(Python3), set Python3_EXECUTABLE to PYTHON_EXE…
3e5471d 
…CUTABLE (TriBITSPub#610)

Change from calling deprecated find_package(PythonInterp) to call
find_package(Python3).  To maintain backward compatibility for external users,
set Python3_EXECUTABLE to PYTHON_EXECUTABLE if the former is unset and the
later is set.

The rest of TriBITS will need to be refactored to absorb this.
@bartlettroscoe
Add refactor script to replace PYTHON_EXECUTABLE with Python3_EXECUTA…
74f1e4c 
…BLE in CMake code (TriBITSPub#610)
@bartlettroscoe
Run refactoring script to-python3.py on all CMake and related files (T…
1ea5a2b 
…riBITSPub#610)

This just renames PYTHON_EXECUTABLE to Python3_EXECUTABLE.
@bartlettroscoe
Manually change PYTHON_EXECUTABLE to Python3_EXECUTABLE (TriBITSPub#610)
a6d8de6 
This was not a token match so it did not match.  I had to fix manually.
@bartlettroscoe
Add build ref section 'Setting or disabling Python' (TriBITSPub#610)
e0eb702 
I can't believe there was never a section on setting or disabling Python from
a TriBITS project user's perspective.  We have tests in TriBITS for the
various behavior and TriBITS project developers users guide doc for the
handling of Python.

Signed-off-by: Roscoe A. Bartlett <[email protected]>
@bartlettroscoe
Adjust clone tests for old Python implementation (TriBITSPub#610)
fa84823 
These tests clone older versions of TribitsExampleProject that are still using
logic to disable the WrapExternal package based on PYTHON_EXECUTABLE.
Therefore, with new TriBITS implementation setting Python3_EXECUTABLE, the old
logic does not allow the enable of WrapExternal and therefore there is one
less test (10 vs. 11).

This commit adjusts the regex for the number of tests being run for that
change.

Once these external repo snapshots are updated, then we will need to revert
this commit (and fix the SHA1s as usual).

Signed-off-by: Roscoe A. Bartlett <[email protected]>
@bartlettroscoe
Move tribits_find_python_interp() to top in clean code ordering (TriB…
15d05c0 
…ITSPub#610)
@bartlettroscoe
Factor tribits_find_python() into smaller macros (TriBITSPub#610)
3958edc 
Just clearning, more self-documenting code
@bartlettroscoe
Add changelog entry for moving to find_package(Python3) (TriBITSPub#610)
c591413
@bartlettroscoe
Copy link
Member Author

CC: @rppawlo

@sebrowne and @achauphan, would someone from the Trilinos Framework team be willing to review this PR in the TriBITS repo before I snapshot this to Trilinos and integrate it there?

@sebrowne
Copy link
Contributor

sebrowne commented Oct 9, 2024

RHEL8 has a default Python version of 3.6.8, so TriBITS projects + Python support will not work there. RHEL9 has a default Python version of 3.9.18 so that's a-okay. We have a large number of RHEL8 platforms, so that has the potential to cause issues. How strong is the requirement for 3.8+?

@bartlettroscoe
Reduce min Python version from 3.8 to 3.6 (TriBITSPub#610)
6fb7d5e 
The default version of `python3` on RHEL8 machines in Python 3.6.8.  (I am not
sure where I got Python 3.8 from.)

But note that RHEL8 machines also seem to have Python 3.11 and 3.12 installed
as well.  I am wondering if it would not be more wise to be pro-active and
push the version of Python being required to a higher version that will be
supported for longer.  (Python 3.6 ended support at the end of 2021.)
@bartlettroscoe
Copy link
Member Author

RHEL8 has a default Python version of 3.6.8, so TriBITS projects + Python support will not work there. RHEL9 has a default Python version of 3.9.18 so that's a-okay. We have a large number of RHEL8 platforms, so that has the potential to cause issues. How strong is the requirement for 3.8+?

It is not a strong requirement at all. Any version of Python 3.x should be fine (and even Python 2.7 is technically is still supported by the code). It is just that even Python 3.8 is at end-of-support this month according to https://devguide.python.org/versions/.

I went ahead and downgrade this to 3.6.8 (see commit 6fb7d5e).

But note the these SNL RHEL8 machines seem to also have Python 3.12.3 installed by default under /usr/bin/python3.12 as well:

$ /usr/bin/python3.12 --version
Python 3.12.3

I am wondering if we should not be more pro-active and push the version of Python3 being used and supported to a more recent version? According to https://devguide.python.org/versions/, Python 3.12 should be supported through 2028. Just a thought.

@bartlettroscoe
Increasing timeout for TriBITS tests from 100 to 200 seconds
3c1e866 
I am not sure what is going on with these machines, but the times for some of
these TriBITS tests when run in parallel has fallen off a cliff.  On my RHEL7
machine last year, most of those most expensive tests would complete in 30sec
or less.
@bartlettroscoe
Copy link
Member Author

FYI: Note that on SNL RHEL8 machines, find_package(Python3) finds Python 3.12, not 3.6 as shown by the TriBITS configure output:

-- Found Python3: /usr/bin/python3.12 (found suitable version "3.12.3", minimum required is "3.6") found components: Interpreter 
-- Python3_EXECUTABLE='/usr/bin/python3.12'

So unless you explicitly configure with -D Python3_EXECUTABLE=$(which python3), people will be using and testing with Python 3.12 anyway.

@bartlettroscoe
Revert "Assume standard Python3/pip3 install of docutils (TriBITSPub#610
701bb48 
)"

This reverts commit 31bfe43.

Turns out that on some RHEL8 machines, 'rst2thml' is installed, but not
'rst2html.py'.  For example, for the hpws machines at SNL, /usr/bin/rst2html
is installed with version:

  $ rst2html --version
  rst2html (Docutils 0.14, Python 3.6.8, on linux)

So this logic looking for 'rst2thml' and 'rst2html.py' is still useful.
@sebrowne
Copy link
Contributor

sebrowne commented Oct 9, 2024

While that is valid, that package is an optional (additional) installation. So the question is "Should this support RHEL8 default Python?" I'm fine with the proposed approach now that you've noticed that Python 3.12 is installed on the RHEL8 machines that we care about the most. It will make us install that package in our UBI8 containers as well, but we can do that.

@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Oct 9, 2024 •
edited
Loading

While that is valid, that package is an optional (additional) installation. So the question is "Should this support RHEL8 default Python?" I'm fine with the proposed approach now that you've noticed that Python 3.12 is installed on the RHEL8 machines that we care about the most. It will make us install that package in our UBI8 containers as well, but we can do that.

I think this is really a decision for the Trilinos Leadership team and close Trilinos stakeholders. TriBITS does not have much of a dependence on Python. (You can build and test your software with just raw CMake/CTest 3.23+.) Python is just gravy for TriBITS.

I will go ahead and merge this.

rabartlett1972
rabartlett1972 approved these changes Oct 9, 2024
View reviewed changes
Copy link
Collaborator

@rabartlett1972 rabartlett1972 left a comment

Choose a reason for hiding this comment

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

@sebrowne approved so I will allow to merge.

@bartlettroscoe bartlettroscoe merged commit bd5ce80 into TriBITSPub:master Oct 9, 2024
6 checks passed
@rabartlett1972 rabartlett1972 mentioned this pull request Oct 10, 2024
Merged
@bartlettroscoe bartlettroscoe mentioned this pull request Oct 10, 2024
Merged
bartlettroscoe added a commit to trilinos/Trilinos that referenced this pull request Oct 10, 2024
@bartlettroscoe
Automatic snapshot commit from tribits at 3dca0964
0817843 
Origin repo remote tracking branch: 'github/master'
Origin repo remote repo URL: 'github = [email protected]:TriBITSPub/TriBITS.git'
Git describe: tribits_start-3502-g28a36351

At commit:

commit 3dca096437acbb7a16b88bee7d9569bccd8e6317
Author:  Samuel E. Browne <[email protected]>
Date:    Thu Jun 6 15:26:15 2024 -0600
Summary: Add Python3 requirement to user guide (#610)

Mostly, this was done to brings in the changes for transitioning to
find_package(Python3) (see TriBITSPub/TriBITS#610).

But the full set of TriBITS PRs merged in were:

* TriBITSPub/TriBITS#608 sebrowne/python3
* TriBITSPub/TriBITS#617 TriBITSPub/610-python3-2
* TriBITSPub/TriBITS#616 bartlettroscoe/610-python3
* TriBITSPub/TriBITS#615 bartlettroscoe/enable-disable-doc-update-2024-08-29
* TriBITSPub/TriBITS#611 bartlettroscoe/tril-13133-TriBITS-Update-License-and-Copyright-2
* TriBITSPub/TriBITS#614 bartlettroscoe/tribits-updates-2024-08-01
* TriBITSPub/TriBITS#613 TriBITSPub/612-improve-tribits-version-error
* TriBITSPub/TriBITS#609 bartlettroscoe/tril-13133-TriBITS-Update-License-and-Copyright
@bartlettroscoe bartlettroscoe mentioned this pull request Oct 11, 2024
Merged
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this pull request Oct 11, 2024
@bartlettroscoe
Automatic snapshot commit from tribits at 3dca0964
e127b7b 
Origin repo remote tracking branch: 'github/master'
Origin repo remote repo URL: 'github = [email protected]:TriBITSPub/TriBITS.git'
Git describe: tribits_start-3502-g28a36351

At commit:

commit 3dca096437acbb7a16b88bee7d9569bccd8e6317
Author:  Samuel E. Browne <[email protected]>
Date:    Thu Jun 6 15:26:15 2024 -0600
Summary: Add Python3 requirement to user guide (trilinos#610)

Mostly, this was done to brings in the changes for transitioning to
find_package(Python3) (see TriBITSPub/TriBITS#610).

But the full set of TriBITS PRs merged in were:

* TriBITSPub/TriBITS#608 sebrowne/python3
* TriBITSPub/TriBITS#617 TriBITSPub/610-python3-2
* TriBITSPub/TriBITS#616 bartlettroscoe/610-python3
* TriBITSPub/TriBITS#615 bartlettroscoe/enable-disable-doc-update-2024-08-29
* TriBITSPub/TriBITS#611 bartlettroscoe/tril-13133-TriBITS-Update-License-and-Copyright-2
* TriBITSPub/TriBITS#614 bartlettroscoe/tribits-updates-2024-08-01
* TriBITSPub/TriBITS#613 TriBITSPub/612-improve-tribits-version-error
* TriBITSPub/TriBITS#609 bartlettroscoe/tril-13133-TriBITS-Update-License-and-Copyright

Signed-off-by: Roscoe A. Bartlett <[email protected]>
@bartlettroscoe bartlettroscoe mentioned this pull request Feb 13, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebrowne sebrowne sebrowne approved these changes

@rabartlett1972 rabartlett1972 rabartlett1972 approved these changes

@achauphan achauphan Awaiting requested review from achauphan

Assignees
No one assigned
Labels
component: core component: documentation component: python_utils type: enhancement
Projects
TriBITS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bartlettroscoe @sebrowne @rabartlett1972