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

Transition from using setup.py to pyproject.toml to specify project metadata #269

Merged
merged 16 commits into from
Feb 29, 2024

Conversation

R-Palazzo
Copy link
Contributor

86azg1hek
Resolve #266

Hi few things to notice/discuss:

  • Docker: I changed setup.py to pyproject.toml
  • setup.cfg: I was not able to convert all the config inside pyproject.toml, mainly:
    • falke8 config
    • The bumpversion (is it ok because we will change it in Issue #267 no?)
    • aliases (don’t no if we’re still using them)

Let me know your thoughts on this, thanks

@R-Palazzo R-Palazzo requested a review from a team as a code owner February 28, 2024 12:37
@R-Palazzo R-Palazzo removed the request for review from a team February 28, 2024 12:38
@R-Palazzo R-Palazzo changed the title Transition from using setup.py to pyroject.toml to specify project metadata Transition from using setup.py to pyproject.toml to specify project metadata Feb 28, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.00%. Comparing base (581e1e7) to head (03c2d87).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #269   +/-   ##
=======================================
  Coverage   48.00%   48.00%           
=======================================
  Files          19       19           
  Lines        1154     1154           
=======================================
  Hits          554      554           
  Misses        600      600           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pyproject.toml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@gsheni
Copy link
Contributor

gsheni commented Feb 28, 2024

  • Regarding the flake8 config, we can switch to ruff in different MR and move the config for that into pyproject.toml
  • Regarding bumpversion, yes okay for now.
  • yes, aliases are not used

@R-Palazzo
Copy link
Contributor Author

Thanks, @gsheni, great advices! I addressed almost all the comments. I wanted to check with you and @amontanez24 about what to do in the update of task.py

tasks.py Outdated
Comment on lines 79 to 90
for line in dependencies:
line = line.strip()
if _validate_python_version(line):
requirement = re.match(r'[^>]*', line).group(0)
requirement = re.sub(r"""['",]""", '', requirement)
version = re.search(r'>=?(\d\.?)+\w*', line).group(0)
if version:
version = re.sub(r'>=?', '==', version)
version = re.sub(r"""['",]""", '', version)
requirement += version

versions.append(requirement)
Copy link
Contributor

@gsheni gsheni Feb 28, 2024

Choose a reason for hiding this comment

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

It might be better to use the Requirement and Specifier class from the packaging library, rather than using regexes. The code might be something like this:

Suggested change
for line in dependencies:
line = line.strip()
if _validate_python_version(line):
requirement = re.match(r'[^>]*', line).group(0)
requirement = re.sub(r"""['",]""", '', requirement)
version = re.search(r'>=?(\d\.?)+\w*', line).group(0)
if version:
version = re.sub(r'>=?', '==', version)
version = re.sub(r"""['",]""", '', version)
requirement += version
versions.append(requirement)
min_requirements = []
for line in dependencies:
min_package = find_min_requirement(line)
min_requirements.append(str(min_package))
min_requirements = "\n".join(min_requirements) + "\n"
from packaging.requirements import Requirement
from packaging.specifiers import Specifier
def find_min_requirement(requirement):
if is_requirement_path(requirement):
# skip requirement paths
# ex '-r core_requirements.txt'
return
requirement = remove_comment(requirement)
if not verify_python_environment(requirement):
return
if ">=" in requirement:
# minimum version specified (ex - 'package >= 0.0.4')
package = Requirement(requirement)
version = find_operator_version(package, ">=")
minimum = create_strict_min(version)
elif "==" in requirement:
# version strictly specified
package = Requirement(requirement)
version = find_operator_version(package, "==")
minimum = create_strict_min(version)
else:
# minimum version not specified (ex - 'package < 0.0.4')
# version not specified (ex - 'package')
raise ValueError(
"Operator does not exist or is an invalid operator (such as '<'). Please specify the minimum version ('>=', '==')."
)
name = determine_package_name(package)
min_requirement = Requirement(name + str(minimum))
return min_requirement
def is_requirement_path(requirement):
if ".txt" in requirement and "-r" in requirement:
return True
return False
def remove_comment(requirement):
if "#" in requirement:
# remove everything after comment character
requirement = requirement.split("#")[0]
return requirement
def determine_package_name(package):
name = package.name
if len(package.extras) > 0:
name = package.name + "[" + package.extras.pop() + "]"
return name
def verify_python_environment(requirement):
package = Requirement(requirement)
if not package.marker:
# no python version specified in requirement
return True
elif package.marker and package.marker.evaluate():
# evaluate --> evaluating the given marker against the current Python process environment
return True
return False

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 I agree with trying to remove regex if possible. One thing I'm wondering that doesn't really come into play in SDGym but might in other libraries is that sometimes we point to a branch directly to install from. If so, would the minimum tests crash there because that syntax doesn't use '<'? An example would be something like

dependencies = [
  "<packagename>@git+<url-to-repo>#egg=<branch or hash>",
]

Copy link
Contributor

@gsheni gsheni left a comment

Choose a reason for hiding this comment

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

Looks good overall. Just 1 suggestion

@@ -22,5 +22,6 @@ jobs:
run: |
python -m pip install --upgrade pip
python -m pip install invoke rundoc .
python -m pip install toml
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong library. It is this one

Suggested change
python -m pip install toml
python -m pip install tomli

tasks.py Outdated
@@ -6,11 +6,14 @@
import re
import shutil
import stat
import sys
from pathlib import Path

import pkg_resources
import toml
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import toml
import tomli

Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

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

:shipit:

@R-Palazzo R-Palazzo merged commit e6890ad into main Feb 29, 2024
45 checks passed
@R-Palazzo R-Palazzo deleted the issue-266-pyproject.toml branch February 29, 2024 18:13
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.

Transition from using setup.py to pyproject.toml to specify project metadata
4 participants