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

add formatting and linting python in pre-commit #370

Merged
merged 7 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/pre-commit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ jobs:
cd $STARTDIR
python3 -m venv /root/pre-commit-venv
source /root/pre-commit-venv/bin/activate
pip install pre-commit
pip install pre-commit ruff
cmake -DENABLE_SIO=ON \
-DCMAKE_CXX_STANDARD=20 \
-DCMAKE_CXX_FLAGS=" -fdiagnostics-color=always -Werror "\
Expand Down
10 changes: 10 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,13 @@ repos:
entry: python ./scripts/updateReadmeLinks.py --check
files: (^README.md$)
language: system
- id: ruff
name: ruff
entry: ruff check --force-exclude
types: [python]
language: system
- id: ruff-format
name: ruff-format
entry: ruff format --force-exclude
types: [python]
language: system
m-fila marked this conversation as resolved.
Show resolved Hide resolved
13 changes: 13 additions & 0 deletions .ruff.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
target-version = "py311"

line-length = 99
Copy link
Contributor

Choose a reason for hiding this comment

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

I was just about to say we should add this. Nice :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@m-fila m-fila Sep 27, 2024

Choose a reason for hiding this comment

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

I tried to be compatible with the black config from k4FWCore https://github.com/key4hep/k4FWCore/blob/main/pyproject.toml since at that moment that was the only python formatting config out there (except for spack I think)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then let's put py310 in k4geo (@tmadlener)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating to py311 here and in k4FWCore and reformatting doesn't change any files. We have python 3.11 in the nightlies. I think we could just update and have py311 everywhere

I can also copy the extra formatting options from k4geo (it's just being explicitly setting the values same as the default right?)

Copy link
Contributor

Choose a reason for hiding this comment

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

it's just being explicitly setting the values same as the default right?

Yes. Specifically the defaults that make ruff work / format the same as black.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could just update and have py311 everywhere

I would be in favor of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated here


[format]
# Make things format the same way as black
quote-style = "double"
indent-style = "space"
skip-magic-trailing-comma = false
line-ending = "auto"

[lint]
select = ["F", "E", "W", "PLE", "PLW", "PLC"]
3 changes: 3 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,6 @@ add_subdirectory(test)

#--- create uninstall target ---------------------------------------------------
include(cmake/EDM4HEPUninstall.cmake)

#--- code format targets -------------------------------------------------------
include(cmake/pythonFormat.cmake)
25 changes: 25 additions & 0 deletions cmake/pythonFormat.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Additional target to run python linters and formatters on python files
#
# Requires ruff to be available in the environment

# Get all our Python files
file(GLOB_RECURSE ALL_PYTHON_FILES ${PROJECT_SOURCE_DIR}/python/*.py
${PROJECT_SOURCE_DIR}/scripts/*.py ${PROJECT_SOURCE_DIR}/test/*.py)

find_program(RUFF_EXECUTABLE ruff)
if(RUFF_EXECUTABLE)
add_custom_target(
ruff
COMMAND ${RUFF_EXECUTABLE}
check --force-exclude ${ALL_PYTHON_FILES}
)
set_target_properties(ruff PROPERTIES EXCLUDE_FROM_ALL TRUE)
add_custom_target(
ruff-format
COMMAND ${RUFF_EXECUTABLE}
format --force-exclude ${ALL_PYTHON_FILES}
)
set_target_properties(ruff-format PROPERTIES EXCLUDE_FROM_ALL TRUE)
else()
message(STATUS "Failed to find ruff executable - no target to run ruff can be set")
endif()
33 changes: 18 additions & 15 deletions python/edm4hep/__init__.py
Original file line number Diff line number Diff line change
@@ -1,34 +1,37 @@
"""Python module for the EDM4HEP datamodel and utilities."""

import sys

from .__version__ import __version__
import ROOT
res = ROOT.gSystem.Load('libedm4hepDict')

res = ROOT.gSystem.Load("libedm4hepDict")
if res < 0:
raise RuntimeError('Failed to load libedm4hepDict')
raise RuntimeError("Failed to load libedm4hepDict")

res = ROOT.gSystem.Load('libedm4hepRDF')
res = ROOT.gSystem.Load("libedm4hepRDF")
if res < 0:
raise RuntimeError('Failed to load libedm4hepRDF')
raise RuntimeError("Failed to load libedm4hepRDF")

res = ROOT.gInterpreter.LoadFile('edm4hep/utils/kinematics.h')
res = ROOT.gInterpreter.LoadFile("edm4hep/utils/kinematics.h")
if res != 0:
raise RuntimeError('Failed to load kinematics.h')
raise RuntimeError("Failed to load kinematics.h")

res = ROOT.gInterpreter.LoadFile('edm4hep/utils/dataframe.h')
res = ROOT.gInterpreter.LoadFile("edm4hep/utils/dataframe.h")
if res != 0:
raise RuntimeError('Failed to load dataframe.h')
raise RuntimeError("Failed to load dataframe.h")

res = ROOT.gInterpreter.LoadFile('edm4hep/Constants.h')
res = ROOT.gInterpreter.LoadFile("edm4hep/Constants.h")
if res != 0:
raise RuntimeError('Failed to load Constants.h')
from ROOT import edm4hep
raise RuntimeError("Failed to load Constants.h")
from ROOT import edm4hep # noqa: E402

from podio.pythonizations import load_pythonizations # noqa: E402

from podio.pythonizations import load_pythonizations
load_pythonizations('edm4hep')
load_pythonizations("edm4hep")

# Make TAB completion work for utils
setattr(edm4hep, 'utils', edm4hep.utils)
setattr(edm4hep, "utils", edm4hep.utils)

# set package attributes for edm4hep
edm4hep.__version__ = __version__
Expand All @@ -38,4 +41,4 @@
edm4hep.__file__ = __file__

# Make `import edm4hep` work
sys.modules['edm4hep'] = edm4hep
sys.modules["edm4hep"] = edm4hep
44 changes: 11 additions & 33 deletions scripts/createEDM4hepFile.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,9 @@ def create_MCParticleCollection():
particle.setCharge(next(counter))
particle.setTime(next(counter))
particle.setMass(next(counter))
particle.setVertex(
edm4hep.Vector3d(next(counter), next(counter), next(counter))
)
particle.setEndpoint(
edm4hep.Vector3d(next(counter), next(counter), next(counter))
)
particle.setMomentum(
edm4hep.Vector3d(next(counter), next(counter), next(counter))
)
particle.setVertex(edm4hep.Vector3d(next(counter), next(counter), next(counter)))
particle.setEndpoint(edm4hep.Vector3d(next(counter), next(counter), next(counter)))
particle.setMomentum(edm4hep.Vector3d(next(counter), next(counter), next(counter)))
particle.setMomentumAtEndpoint(
edm4hep.Vector3d(next(counter), next(counter), next(counter))
)
Expand Down Expand Up @@ -174,9 +168,7 @@ def create_ClusterCollection(vectorsize, calo_hit):
cluster.setPositionError(create_CovMatrixNf(3))
cluster.setITheta(next(counter))
cluster.setPhi(next(counter))
cluster.setDirectionError(
edm4hep.Vector3f(next(counter), next(counter), next(counter))
)
cluster.setDirectionError(edm4hep.Vector3f(next(counter), next(counter), next(counter)))
for j in range(vectorsize):
cluster.addToShapeParameters(next(counter))
cluster.addToSubdetectorEnergies(next(counter))
Expand Down Expand Up @@ -256,9 +248,7 @@ def create_TrackCollection(vectorsize, tracker_hit):
state.Z0 = next(counter)
state.tanLambda = next(counter)
state.time = next(counter)
state.referencePoint = edm4hep.Vector3f(
next(counter), next(counter), next(counter)
)
state.referencePoint = edm4hep.Vector3f(next(counter), next(counter), next(counter))
state.covMatrix = create_CovMatrixNf(6)
track.addToTrackStates(state)

Expand Down Expand Up @@ -292,9 +282,7 @@ def create_ReconstructedParticleCollection(vertex, cluster, track):
rparticle.setPDG(next(counter))
rparticle.setEnergy(next(counter))
rparticle.setMomentum(edm4hep.Vector3f(next(counter), next(counter), next(counter)))
rparticle.setReferencePoint(
edm4hep.Vector3f(next(counter), next(counter), next(counter))
)
rparticle.setReferencePoint(edm4hep.Vector3f(next(counter), next(counter), next(counter)))
rparticle.setCharge(next(counter))
rparticle.setMass(next(counter))
rparticle.setGoodnessOfPID(next(counter))
Expand Down Expand Up @@ -402,9 +390,7 @@ def create_frame():
particles = frame.put(create_MCParticleCollection(), "MCParticleCollection")
particle = particles[0]

hits = frame.put(
create_SimTrackerHitCollection(particle), "SimTrackerHitCollection"
)
hits = frame.put(create_SimTrackerHitCollection(particle), "SimTrackerHitCollection")
simtracker_hit = hits[0]

calo_contribs = frame.put(
Expand All @@ -422,9 +408,7 @@ def create_frame():
hits = frame.put(create_CalorimeterHitCollection(), "CalorimeterHitCollection")
calo_hit = hits[0]

clusters = frame.put(
create_ClusterCollection(VECTORSIZE, calo_hit), "ClusterCollection"
)
clusters = frame.put(create_ClusterCollection(VECTORSIZE, calo_hit), "ClusterCollection")
cluster = clusters[0]

hits = frame.put(create_TrackerHit3DCollection(), "TrackerHit3DCollection")
Expand All @@ -433,9 +417,7 @@ def create_frame():

frame.put(create_RawTimeSeriesCollection(VECTORSIZE), "RawTimeSeriesCollection")

tracks = frame.put(
create_TrackCollection(VECTORSIZE, tracker_hit), "TrackCollection"
)
tracks = frame.put(create_TrackCollection(VECTORSIZE, tracker_hit), "TrackCollection")
track = tracks[0]

pids, pid = create_ParticleIDCollection(VECTORSIZE)
Expand All @@ -456,9 +438,7 @@ def create_frame():

put_link_collection(frame, "RecoMCParticleLink", reco_particle, particle)
put_link_collection(frame, "CaloHitSimCaloHitLink", calo_hit, simcalo_hit)
put_link_collection(
frame, "TrackerHitSimTrackerHitLink", tracker_hit, simtracker_hit
)
put_link_collection(frame, "TrackerHitSimTrackerHitLink", tracker_hit, simtracker_hit)
put_link_collection(frame, "CaloHitMCParticleLink", calo_hit, particle)
put_link_collection(frame, "ClusterMCParticleLink", cluster, particle)
put_link_collection(frame, "TrackMCParticleLink", track, particle)
Expand All @@ -481,9 +461,7 @@ def create_frame():
parser.add_argument(
"--rntuple", action="store_true", help="Use a ROOT ntuple instead of EDM4hep"
)
parser.add_argument(
"--output-file", type=str, help="Output file name", default="edm4hep.root"
)
parser.add_argument("--output-file", type=str, help="Output file name", default="edm4hep.root")
args = parser.parse_args()
output_file = args.output_file

Expand Down
96 changes: 60 additions & 36 deletions scripts/updateReadmeLinks.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,46 +5,70 @@

parser = argparse.ArgumentParser()
parser.add_argument("inputfile", help="The README file to run on", default="README.md")
parser.add_argument("--check", action='store_true', help='Use if you want to just check the links without actually changing the file')
parser.add_argument(
"--check",
action="store_true",
help="Use if you want to just check the links without actually changing the file",
)
args = parser.parse_args()


def check_readme_links():
edm4hep_yaml_content = ""
with open("edm4hep.yaml") as edm4hep_yaml:
edm4hep_yaml_content = edm4hep_yaml.readlines()

readme_content = ""
with open("README.md", "r") as readme:
readme_content = readme.readlines()

new_readme_content = ""

links_are_ok = True
for readme_line in readme_content:
edm4hep_objects = re.findall("\[(.*?)\]\(https:\/\/github.com\/key4hep\/EDM4hep\/blob\/main\/edm4hep\.yaml#L(\d+?)\)", readme_line)
if edm4hep_objects:
for edm4hep_object, original_line_number in edm4hep_objects:
regex = f".*edm4hep::{edm4hep_object} *: *"
# find in which line it appears in edm4hep.yaml
edm4hep_yaml_line_numbers_with_match = [str(i + 1) for i, line in enumerate(edm4hep_yaml_content) if re.match(regex, line)]
if len(edm4hep_yaml_line_numbers_with_match) != 1:
print(f"Error: failed to replace line number for {edm4hep_object}, either no or several matches were found in edm4hep.yaml with the regex '{regex}'")
sys.exit(1)
if edm4hep_yaml_line_numbers_with_match[0] != original_line_number:
links_are_ok = False
readme_line = readme_line.replace(f"[{edm4hep_object}](https://github.com/key4hep/EDM4hep/blob/main/edm4hep.yaml#L{original_line_number})", f"[{edm4hep_object}](https://github.com/key4hep/EDM4hep/blob/main/edm4hep.yaml#L{edm4hep_yaml_line_numbers_with_match[0]})")
print(f"{edm4hep_object} is wrongly linked (line {original_line_number} --> {edm4hep_yaml_line_numbers_with_match[0]})")
new_readme_content += readme_line
return links_are_ok, new_readme_content
edm4hep_yaml_content = ""
with open("edm4hep.yaml") as edm4hep_yaml:
edm4hep_yaml_content = edm4hep_yaml.readlines()

readme_content = ""
with open("README.md", "r") as readme:
readme_content = readme.readlines()

new_readme_content = ""

links_are_ok = True
for readme_content_line in readme_content:
readme_line = readme_content_line
edm4hep_objects = re.findall(
r"\[(.*?)\]\(https:\/\/github.com\/key4hep\/EDM4hep\/blob\/main\/edm4hep\.yaml#L(\d+?)\)",
readme_line,
)
if edm4hep_objects:
for edm4hep_object, original_line_number in edm4hep_objects:
regex = f".*edm4hep::{edm4hep_object} *: *"
# find in which line it appears in edm4hep.yaml
edm4hep_yaml_line_numbers_with_match = [
str(i + 1)
for i, line in enumerate(edm4hep_yaml_content)
if re.match(regex, line)
]
if len(edm4hep_yaml_line_numbers_with_match) != 1:
print(
f"Error: failed to replace line number for {edm4hep_object}, either no "
f"or several matches were found in edm4hep.yaml with the regex '{regex}'"
)
sys.exit(1)
if edm4hep_yaml_line_numbers_with_match[0] != original_line_number:
links_are_ok = False
readme_line = readme_line.replace(
f"[{edm4hep_object}](https://github.com/key4hep/EDM4hep/blob/main/edm4hep.yaml#L{original_line_number})",
f"[{edm4hep_object}](https://github.com/key4hep/EDM4hep/blob/main/edm4hep.yaml#L{edm4hep_yaml_line_numbers_with_match[0]})",
)
print(
f"{edm4hep_object} is wrongly linked (line {original_line_number} --> {edm4hep_yaml_line_numbers_with_match[0]})" # noqa: E501
)
new_readme_content += readme_line
return links_are_ok, new_readme_content


links_are_ok, new_readme_content = check_readme_links()
if links_are_ok:
print("README.md links are fine, nothing to change")
print("README.md links are fine, nothing to change")
else:
if not args.check:
with open("README.md", "w") as readme:
readme.write(new_readme_content)
print("README.md links updated.")
else:
print("README.md links should be updated. (Run ./scripts/updateReadmeLinks.py to fix them)")
sys.exit(1)
if not args.check:
with open("README.md", "w") as readme:
readme.write(new_readme_content)
print("README.md links updated.")
else:
print(
"README.md links should be updated. (Run ./scripts/updateReadmeLinks.py to fix them)"
)
sys.exit(1)
2 changes: 1 addition & 1 deletion test/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ def pytest_addoption(parser):
def pytest_configure(config):
"""This is a slighty hacky solution to make the pytest configuration
available in test modules outside of fixtures"""
global options
global options # noqa: PLW0603
options = config.option
Loading
Loading