Use permalinks in ecosystem diff references #5704

zanieb · 2023-07-12T05:02:02Z

Summary

cc @dhruvmanila as I see you assigned yourself to the issue

Test Plan

Ran locally e.g.

<a href='https://github.com/zulip/zulip/blob/fcede324202ac5bc3de0691d2d6e168d0f18c120/zproject/prod_settings_template.py#L144'>zproject/prod_settings_template.py:144:26:</a>

Should succeed in CI as well.

zanieb · 2023-07-12T05:06:41Z

scripts/check_ecosystem.py

-            yield Path(checkout_dir)
+            yield await self._get_commit(checkout_dir)


So... because Repository is immutable (and I liked that it was immutable) we can't just get the commit SHA on clone and stash it on the Repository object. Instead, we yield it and it's attached to the Diff object for later display. It also turns out that the previously yielded Path was identical to the input argument so I removed that.

We probably ought to just have the ecosystem checks pinned to a specific SHA that's updated on a schedule by a bot and then we can skip the retrieval and just assign it to the Repository at creation time. That seems pretty low priority though.

Also happy to hear alternative approaches if anyone has ideas! This seemed liked the simplest way to retain immutability but this was my first time looking at this script.

We could replace the value:

self._replace(ref=await self._get_commit(checkout_dir))

But, otherwise I think this is fine.

I'm not sure how we'd get the new Repository object back to the relevant owner without a bunch of changes.

Ah yeah, didn't think about that (_replace returns a new object). I think this is fine then. We can iterate it later.

zanieb · 2023-07-12T05:07:14Z

scripts/check_ecosystem.py

-        process = await create_subprocess_exec(
-            *git_command,
+        git_clone_process = await create_subprocess_exec(
+            *git_clone_command,


I renamed these before extracting _get_commit into a separate function. I don't think the extra clarity hurts though.

dhruvmanila · 2023-07-12T05:06:24Z

scripts/check_ecosystem.py

+    async def _get_commit(self: Self, checkout_dir: Path) -> str:
+        """Return the commit sha for the repository in the checkout directory."""
+        git_sha_process = await create_subprocess_exec(
+            *["git", "rev-parse", "head"],


nit

Suggested change

*["git", "rev-parse", "head"],

*["git", "rev-parse", "HEAD"],

Interestingly this worked locally but failed with ambiguous argument 'head': unknown revision or path not in the working tree in CI — curious to see if this changes anything but I doubt it?

Failure at https://github.com/astral-sh/ruff/actions/runs/5527736701/jobs/10083885033

Interesting! https://stackoverflow.com/a/56346962

That resolved it :)

dhruvmanila · 2023-07-12T05:11:50Z

scripts/check_ecosystem.py

@@ -45,11 +45,11 @@ async def clone(self: Self, checkout_dir: Path) -> AsyncIterator[Path]:
        """Shallow clone this repository to a temporary directory."""
        if checkout_dir.exists():
            logger.debug(f"Reusing {self.org}:{self.repo}")
-            yield Path(checkout_dir)
+            yield await self._get_commit(checkout_dir)


Can we add the commit SHA to the Repository object as that's where I think it should be logically? I think adding it to ref should be the same meaning as branches are just pointers to specific commits.

This will also mean that the repo.url_for function/call need not be changed.

I tried that first but we cannot because it's immutable. We definitely could decide to make it mutable, but I liked the immutable design. More commentary at #5704 (comment)

Oh, I missed that comment. Give me a minute.

Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>

scripts/check_ecosystem.py

dhruvmanila · 2023-07-12T05:38:09Z

Oh shoot! Sorry, I fixed the wrong long line :p

zanieb · 2023-07-12T05:39:11Z

No problem :)

konstin · 2023-07-12T05:49:08Z

scripts/check_ecosystem.py

        if lnum:
            url += f"#L{lnum}"
        return url

+    async def _get_commit(self: Self, checkout_dir: Path) -> str:
+        """Return the commit sha for the repository in the checkout directory."""
+        git_sha_process = await create_subprocess_exec(


you can use subprocess.check_output with text=True here, it will also check the output status automatically

ruff/scripts/update_schemastore.py

Line 18 in 0666add

check_output(["git", "rev-parse", "--show-toplevel"], text=True).strip(),

Hm I'd rather not make a blocking call in an async function though 👿

github-actions · 2023-07-12T05:49:22Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.02      8.0±0.04ms     5.1 MB/sec    1.00      7.9±0.01ms     5.2 MB/sec
formatter/numpy/ctypeslib.py               1.01   1874.2±4.39µs     8.9 MB/sec    1.00   1857.7±3.28µs     9.0 MB/sec
formatter/numpy/globals.py                 1.01    208.7±0.42µs    14.1 MB/sec    1.00    207.3±0.52µs    14.2 MB/sec
formatter/pydantic/types.py                1.03      4.1±0.02ms     6.3 MB/sec    1.00      4.0±0.03ms     6.4 MB/sec
linter/all-rules/large/dataset.py          1.01     13.7±0.17ms     3.0 MB/sec    1.00     13.6±0.24ms     3.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      3.4±0.02ms     4.9 MB/sec    1.00      3.4±0.04ms     4.9 MB/sec
linter/all-rules/numpy/globals.py          1.01    433.1±0.94µs     6.8 MB/sec    1.00    429.4±3.15µs     6.9 MB/sec
linter/all-rules/pydantic/types.py         1.01      6.1±0.08ms     4.2 MB/sec    1.00      6.0±0.09ms     4.2 MB/sec
linter/default-rules/large/dataset.py      1.01      6.8±0.04ms     6.0 MB/sec    1.00      6.7±0.07ms     6.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1473.1±2.64µs    11.3 MB/sec    1.00   1457.8±2.44µs    11.4 MB/sec
linter/default-rules/numpy/globals.py      1.02    168.6±0.25µs    17.5 MB/sec    1.00    164.7±0.26µs    17.9 MB/sec
linter/default-rules/pydantic/types.py     1.02      3.1±0.04ms     8.3 MB/sec    1.00      3.0±0.01ms     8.4 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00     12.5±0.74ms     3.2 MB/sec    1.01     12.7±0.56ms     3.2 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.8±0.16ms     5.8 MB/sec    1.00      2.8±0.19ms     5.9 MB/sec
formatter/numpy/globals.py                 1.00   322.7±27.64µs     9.1 MB/sec    1.02   328.1±25.67µs     9.0 MB/sec
formatter/pydantic/types.py                1.00      6.4±0.42ms     4.0 MB/sec    1.00      6.3±0.40ms     4.0 MB/sec
linter/all-rules/large/dataset.py          1.00     21.7±1.01ms  1918.4 KB/sec    1.03     22.3±0.91ms  1865.5 KB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      5.7±0.28ms     2.9 MB/sec    1.00      5.6±0.32ms     3.0 MB/sec
linter/all-rules/numpy/globals.py          1.00   672.7±51.92µs     4.4 MB/sec    1.01   679.1±42.70µs     4.3 MB/sec
linter/all-rules/pydantic/types.py         1.03     10.0±0.57ms     2.6 MB/sec    1.00      9.7±0.46ms     2.6 MB/sec
linter/default-rules/large/dataset.py      1.00     10.9±0.57ms     3.7 MB/sec    1.00     10.9±0.62ms     3.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.02      2.4±0.27ms     7.1 MB/sec    1.00      2.3±0.34ms     7.2 MB/sec
linter/default-rules/numpy/globals.py      1.00   270.8±19.50µs    10.9 MB/sec    1.01   274.8±20.89µs    10.7 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.9±0.29ms     5.2 MB/sec    1.02      5.0±0.28ms     5.1 MB/sec

Closes #5702

Use permalinks in ecosystem diff references

512cb5d

zanieb commented Jul 12, 2023

View reviewed changes

zanieb requested review from konstin and dhruvmanila July 12, 2023 05:07

dhruvmanila reviewed Jul 12, 2023

View reviewed changes

Use HEAD instead of head

43bc3cd

Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>

zanieb commented Jul 12, 2023

View reviewed changes

scripts/check_ecosystem.py Outdated Show resolved Hide resolved

Fixups

a98f9b4

dhruvmanila approved these changes Jul 12, 2023

View reviewed changes

dhruvmanila reviewed Jul 12, 2023

View reviewed changes

scripts/check_ecosystem.py Show resolved Hide resolved

dhruvmanila and others added 3 commits July 12, 2023 11:05

Split long lines

63b35e3

Fix line length

9d7cd84

Restore long line :p

7c1979f

konstin approved these changes Jul 12, 2023

View reviewed changes

zanieb merged commit 33a9177 into main Jul 12, 2023
16 checks passed

zanieb deleted the ci/ecosystem-permalinks branch July 12, 2023 06:26

konstin pushed a commit that referenced this pull request Jul 19, 2023

Use permalinks in ecosystem diff references (#5704)

0317ee1

Closes #5702

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use permalinks in ecosystem diff references #5704

Use permalinks in ecosystem diff references #5704

zanieb commented Jul 12, 2023 •

edited

zanieb Jul 12, 2023 •

edited

zanieb Jul 12, 2023

dhruvmanila Jul 12, 2023

zanieb Jul 12, 2023

dhruvmanila Jul 12, 2023

zanieb Jul 12, 2023

dhruvmanila Jul 12, 2023

zanieb Jul 12, 2023 •

edited

zanieb Jul 12, 2023

dhruvmanila Jul 12, 2023

zanieb Jul 12, 2023

dhruvmanila Jul 12, 2023

dhruvmanila Jul 12, 2023

zanieb Jul 12, 2023

dhruvmanila Jul 12, 2023

dhruvmanila commented Jul 12, 2023 •

edited

zanieb commented Jul 12, 2023

konstin Jul 12, 2023

zanieb Jul 12, 2023

konstin Jul 12, 2023

github-actions bot commented Jul 12, 2023 •

edited

		yield Path(checkout_dir)
		yield await self._get_commit(checkout_dir)

	*["git", "rev-parse", "head"],
	*["git", "rev-parse", "HEAD"],

Use permalinks in ecosystem diff references #5704

Use permalinks in ecosystem diff references #5704

Conversation

zanieb commented Jul 12, 2023 • edited

Summary

Test Plan

zanieb Jul 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanieb Jul 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvmanila commented Jul 12, 2023 • edited

zanieb commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 12, 2023 • edited

PR Check Results

Ecosystem

Benchmark

Linux

Windows

zanieb commented Jul 12, 2023 •

edited

zanieb Jul 12, 2023 •

edited

zanieb Jul 12, 2023 •

edited

dhruvmanila commented Jul 12, 2023 •

edited

github-actions bot commented Jul 12, 2023 •

edited