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

Port Field to Rust #19143

Merged
merged 4 commits into from
Jun 1, 2023
Merged

Port Field to Rust #19143

merged 4 commits into from
Jun 1, 2023

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented May 24, 2023

This change ports Field to Rust.

One important takeaway was that pyo3 gives us less of an advantage when subclassing is in use, because most methods of the class need to accept themselves as PyCell or PyRef (in order to gain access to their subclass instance, which isn't a member of the wrapped struct). Those methods can then only be called via pyo3 as well.

Despite that, I'll likely still port Target to Rust for consistency, and because it has some longer methods which I expect to see actual performance benefit from porting.

@stuhood stuhood added the category:internal CI, fixes for not-yet-released features, etc. label May 24, 2023
@stuhood stuhood force-pushed the stuhood/field-to-rust branch 3 times, most recently from bd168c2 to 000f6af Compare May 25, 2023 16:36
@stuhood stuhood marked this pull request as ready for review May 25, 2023 16:53
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks! Any benefit to porting the Field subclasses like BoolField?

@stuhood
Copy link
Member Author

stuhood commented May 26, 2023

Thanks! Any benefit to porting the Field subclasses like BoolField?

Possibly, but not in the interest of making porting graph.py easier, I don't think. The Rust code won't ever need to interact with the subclasses.

@stuhood stuhood enabled auto-merge (squash) May 26, 2023 16:39
@stuhood
Copy link
Member Author

stuhood commented May 31, 2023

Friendly ping.

@thejcannon thejcannon disabled auto-merge May 31, 2023 19:09
@thejcannon thejcannon enabled auto-merge (squash) May 31, 2023 19:10
@thejcannon
Copy link
Member

I'll take a look ASAP, although @huonw might have better review goggles than the Joshs 😄

Any back-of-the-envelope perf numbers to share?

@thejcannon thejcannon disabled auto-merge June 1, 2023 15:13
Copy link
Member

@thejcannon thejcannon left a comment

Choose a reason for hiding this comment

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

I didn't look too closely at the Rust code. I figure the perf has to be better just from porting. And if the tests pass that's a good indicator of code soundness.

Comment on lines +257 to +259
# Type alias to express the intent that the type should be immutable and hashable. There's nothing
# to actually enforce this, outside of convention.
ImmutableValue = Any
Copy link
Member

Choose a reason for hiding this comment

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

FYI The Python std library (and therefore core devs) have roughly said that hashability implies immutability. Search https://docs.python.org/3/library/dataclasses.html for "mutability".

So in that regard, this could be an alias for typing.Hashable

Copy link
Member Author

@stuhood stuhood Jun 1, 2023

Choose a reason for hiding this comment

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

That's potentially backwards incompatible... but assuming that mypy doesn't misbehave, it should only catch bugs? Will try it.

Copy link
Member Author

Choose a reason for hiding this comment

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

That causes a number of type checking errors in our code (and more in user code most likely): will save for another change.

Copy link
Member

Choose a reason for hiding this comment

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

What were the errors? That's concerning...

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't look into it, but there were at least 10.

My goal with these port PRs is not to change behavior (unless it makes it easier to accomplish them), since my feeling right now is that getting out of our performance hole is the most important goal.

with pytest.raises(FrozenInstanceError):
with pytest.raises(AttributeError):
Copy link
Member

Choose a reason for hiding this comment

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

oh bummer...

@stuhood stuhood force-pushed the stuhood/field-to-rust branch from 000f6af to 8b22bb6 Compare June 1, 2023 17:09
@stuhood stuhood enabled auto-merge (squash) June 1, 2023 17:09
@stuhood
Copy link
Member Author

stuhood commented Jun 1, 2023

Any back-of-the-envelope perf numbers to share?

No, haven't done any benchmarking... I'm really not expecting anything here though, given how tiny the various functions were.

@thejcannon thejcannon disabled auto-merge June 1, 2023 17:34
@thejcannon
Copy link
Member

(FYI force-pushing means I can't review just what's changed since my last review. Usually in order to not overwhelm myself I trust past-Josh's review of the code and just review the diff. In this instance I have no clue what changed since the last review 😢 )

@stuhood
Copy link
Member Author

stuhood commented Jun 1, 2023

(FYI force-pushing means I can't review just what's changed since my last review. Usually in order to not overwhelm myself I trust past-Josh's review of the code and just review the diff. In this instance I have no clue what changed since the last review 😢 )

Nothing has changed since your last review (for the reasons in the threads): if anything had, I would have created a separate commit.

@stuhood stuhood force-pushed the stuhood/field-to-rust branch from 8b22bb6 to 1c16922 Compare June 1, 2023 19:56
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Nice!

@stuhood stuhood enabled auto-merge (squash) June 1, 2023 20:56
@stuhood stuhood merged commit d97e4ef into pantsbuild:main Jun 1, 2023
@stuhood stuhood deleted the stuhood/field-to-rust branch June 1, 2023 21:19
@wisechengyi wisechengyi mentioned this pull request Jun 3, 2023
Comment on lines +112 to +113
// TODO: Should be `RequiredFieldMissingException`.
Err(PyValueError::new_err(format!(
Copy link
Member

Choose a reason for hiding this comment

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

Is using an error defined in python more difficult than using the built in errors? This is the only conerning thing I see in this PR (from my post-review merge). Very cool stuff!

wisechengyi added a commit that referenced this pull request Jun 4, 2023
### Internal

* upgrade to Rust v1.70.0
([#19228](#19228))

* Remove the last mentions of NO_TOOL_LOCKFILE.
([#19229](#19229))

* Upgrade Helm unittest
([#19220](#19220))

* Prepare `2.17.0rc0`.
([#19226](#19226))

* Use a trait for remote action result caching
([#19097](#19097))

* Port `Field` to Rust
([#19143](#19143))

* Upgrade `pyo3` to `0.19`.
([#19223](#19223))

* Prepare `2.16.0rc5`.
([#19221](#19221))

* internal: Create dep inference request type
([#19001](#19001))

* Avoid requiring Python when trying to install Python (using Pyenv)
([#19208](#19208))

* Fix secondary ownership warning semantics
([#19191](#19191))

* Bump os_pipe from 1.1.3 to 1.1.4 in /src/rust/engine
([#19202](#19202))

* Include committer date in local version identifier of unstable builds
([#19179](#19179))

* Ensure we set the AWS region.
([#19178](#19178))
benjyw added a commit that referenced this pull request Oct 12, 2023
Field construction was ported to Rust in #19143, which incidentally
broke the field deprecation check. As a result, a deprecation warning
was emitted even if the user was not setting the field explicitly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants