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

Tweak V2 pytest's execute process description to get more cache hits #7737

Merged

Conversation

Projects
None yet
4 participants
@Eric-Arellano
Copy link
Contributor

commented May 15, 2019

Resolves #7732. The resolve requirements process is solely a function of the requirements it is fed and does not matter what target it is being used for.

As followups, we will likely want to:

  1. No longer make description be a part of the cache key.
  2. Improve visualization of V2 rules, both for the rule graph and for the UI.

@Eric-Arellano Eric-Arellano requested review from stuhood and illicitonion May 15, 2019

@stuhood
Copy link
Member

left a comment

This will probably be long... I expect that wherever we render those we will need to account for that anyway. Should give @illicitonion a chance to look before merging probably.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

This will probably be long... I expect that wherever we render those we will need to account for that anyway.

Agreed. Today was my first time using the visualization, and one of the things I immediately wanted to change was how long lines don't wrap. Makes the PDF extremely wide. This node would now suffer from the same issue.

I would be fine with making the description be "Resolving all 3rd party and Pytest dependencies" no matter what. I think the argv field should already include the list of requirements, so this current description arguably duplicates information.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

@Eric-Arellano #7024 is a previous attempt at improving the graph visualization that I haven't quite made mergeable yet for absolutely no good reason other than I got distracted thinking about d3 and interactivity.

@illicitonion
Copy link
Contributor

left a comment

Let's merge this as-is.

Two thoughts:

  1. If we need to address display things, we're going to need to do that in a general way at some point; there are two consumers, the UI and the visualization traces; these should probably handle this (or we should strictly impose some constraints on descriptions). Let's not worry too much about it for now.
  2. I'd be open to making description not part of the cache key. It doesn't affect the actual execution or result, only logging around it.
@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

I'd be open to making description not part of the cache key. It doesn't affect the actual execution or result, only logging around it.

That sounds like a good change to me. If it isn't too hard to do and I get a little direction, would be interested in taking this on to learn better how Rust code works and how cache keys work. If it will take more than two hours, though, probably better for me to stay focused.

@Eric-Arellano Eric-Arellano merged commit 27a3819 into pantsbuild:master May 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:v2-pytest-pex-description branch May 16, 2019

@illicitonion

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

I'd be open to making description not part of the cache key. It doesn't affect the actual execution or result, only logging around it.

That sounds like a good change to me. If it isn't too hard to do and I get a little direction, would be interested in taking this on to learn better how Rust code works and how cache keys work. If it will take more than two hours, though, probably better for me to stay focused.

Filed #7745 - feel free to give it a go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.