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

Move to Poetry for Python #1586

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Move to Poetry for Python #1586

wants to merge 15 commits into from

Conversation

taddes
Copy link
Contributor

@taddes taddes commented Jul 15, 2024

Description

Update to Poetry for Python dependency management.

This puts all the tools, dependencies, and commands in place. However, we won't spin our wheels right now converting over in CI, moving from venv and whatnot, since we're not prioritizing Sync modernization atm. We will address down the road, but for the time being increases developer speed and experience for checks and makes some improvements to the python utilities.

Benefits:

  • Simpler management of scoped dependencies within subdirectories
  • Faster build times
  • Better auditing of out-of-date packages
  • Modern Python standard

Testing

How should reviewers test?

Issue(s)

Closes 4293.

@taddes taddes self-assigned this Jul 15, 2024
[tool.poetry]
name = "syncstorage-rs"
version = "0.1.0"
description = "sync storage server in rust"
Copy link
Member

Choose a reason for hiding this comment

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

There's a potential that folk might discover this package while looking for a stand-alone Sync storage server. Let's be exceptionally clear about what this is and how it's being used.

Suggested change
description = "sync storage server in rust"
description = "Support package for the Mozilla Sync storage server, written in rust. This is not a 'stand alone' server and is not meant for general use to run your own Sync storage server. Please see [the wiki page](https://github.com/mozilla-services/syncstorage-rs/wiki/overview) for further details."

pyproject.toml Outdated Show resolved Hide resolved
@taddes taddes force-pushed the SYNC-4293_poetry_deps_mgmt branch from b8c0ff2 to 2aa78e2 Compare July 18, 2024 18:57
@taddes taddes changed the title WIP: Move to Poetry for Python Move to Poetry for Python Sep 4, 2024
@taddes taddes marked this pull request as ready for review September 4, 2024 19:52
)
writer = DataFileWriter(
open(out_file_name, "wb"), DatumWriter(), schema)
for chunk in range(max(1, math.trunc(math.ceil(row_count / MAX_ROWS)))):
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, i'd have words with the folk behind black and why they want python to look like perl, but 🤷🏻

""" group by userid order by userid""".format(
bso_num))
sql = (
"""select userid, count(*) as count from bso{}"""
Copy link
Member

Choose a reason for hiding this comment

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

Your call, but we could probably reformat this to make it more "black" like:

Suggested change
"""select userid, count(*) as count from bso{}"""
f"""select userid, count(*) as count from bso{bso_num}"""
""" group by userid order by userid"""

result = database.execute_partitioned_dml(query, params=params, param_types=param_types)
result = database.execute_partitioned_dml(
query, params=params, param_types=param_types
)
end = datetime.now()
logging.info(
"{name}: removed {result} rows, {name}_duration: {time}, prefix: {prefix}".format(
Copy link
Member

Choose a reason for hiding this comment

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

Again, purely optional, but we could redo this as an f"" string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Just noting that given this change creates a lot of formatting changes, I tried to limit them best I could and get the tools in place to help, all while at least respecting flake8. We can tackle the remainder of these when we are past the priority issues, but I'll add this one in.

@@ -87,35 +86,24 @@ def purge_old_records(
range_msg = ""
if uid_range:
range_msg = (
f" within range {uid_range[0] or 'Start'}"
f" to {uid_range[1] or 'End'}"
f" within range {uid_range[0] or 'Start'}" f" to {uid_range[1] or 'End'}"
Copy link
Member

Choose a reason for hiding this comment

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

Ok, odd format.

Let's just join these two f"" strings and maybe we can drop the parens?

@taddes
Copy link
Contributor Author

taddes commented Sep 19, 2024

Thanks for the comments @jrconlin . I shelved this one before MozWeek, but I don't think it hurts to merge in now, we can pick up the steps later once we've finished our priority tasks. All the parts are there to utilize the tooling with some format improvements, we'll just make the move to Poetry (officially, should we wish) down the road.

@jrconlin jrconlin marked this pull request as draft September 19, 2024 22:44
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