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

Revert "Compile AgML under python 3" #726

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

klendathu2k
Copy link
Contributor

Reverts #646

This remains a draft PR. It modifies AgML to use python 3 syntax which potentially introduces breaking changes if it is used with python 2.7.

Copy link
Contributor

@genevb genevb left a comment

Choose a reason for hiding this comment

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

I'm confused. This PR says it "modifies AgML to use python 3 syntax", implying it is intended to allow AgML to work with python 3. This PR also states that it "reverts #646 ", itself a PR which says it is "for compiling AgML using python 3". Is this PR then an alternative approach to getting AgML to work with python 3 in some way?

I understand that python does allow for version-checking at run-time, e.g. sys.version_info >= (3,0) or something close to that. So unless there is a syntax change that prevents the compilation before running, one could perhaps leverage that for version dependencies?

@klendathu2k
Copy link
Contributor Author

I did not do any version checking to handle the 2.7 --> 3.0 changes. So once we merge PR #646, AgML is guaranteed to run correctly only under python 3+. I cannot guarantee that the geometries will be correct if the code in PR #646 is run with python 2.7. This is why it is a draft PR.

I'm surprised that (1) people found / reviewed the code, (2) the code doesn't crash in the CI tests and (3) that github even lets us merge a draft.

This PR will undo the change. The code will revert back to the python 2.7 syntax, and will be consistent with the build system on SL7.

Alternatively, we could switch to python 3 in our production environment. This might be a better option.

@genevb
Copy link
Contributor

genevb commented Jan 23, 2025 via email

@genevb
Copy link
Contributor

genevb commented Jan 23, 2025

Notes on the available python versions I see at SDCC:

SL7 interactive nodes:
2.7.18 <= Will allow us to re-compile existing libraries, and currently to what "python" soft-links
3.6.8 <= Will allow us to move forward with py3 on SL7
3.8.13 <= Currently to what "python3" soft-links

SL7 container on Alma9 interactive nodes:
2.7.5 <= Will allow us to re-compile existing libraries, and currently to what "python" soft-links
3.6.8 <= Will allow us to move forward with py3 on SL7, and currently to what "python3" soft links

Alma9 interactive nodes:
3.9.16 <= Once we know py3 works on SL7, we can begin looking at compiling natively on Alma9

Hopefully no surprises between the sub-versions of py2 and py3...

-Gene

@klendathu2k
Copy link
Contributor Author

klendathu2k commented Jan 23, 2025 via email

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.

3 participants