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

remove dependency matrix from asv.conf.json #113

Merged
merged 9 commits into from
Apr 18, 2024

Conversation

zacharyburnett
Copy link
Contributor

@zacharyburnett zacharyburnett commented Apr 11, 2024

why are these packages in the matrix in the first place? Would it be okay if we removed them?

I added a set_asv_dependencies script to the bench repository so we can add dependencies on the fly when starting a new benchmark run; otherwise, as they are currently, this matrix just defaults to the latest versions of these dependencies.

Additionally, the format of the configuration is deprecated; it should be under matrix.req, not matrix

@zacharyburnett zacharyburnett changed the title use req key in asv.conf.json matrix remove dependency matrix from asv.conf.json Apr 11, 2024
@pllim
Copy link
Member

pllim commented Apr 12, 2024

why are these packages in the matrix in the first place?

Looks like @MridulS added them in #111 recently?

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Apr 12, 2024

for example, the benchmarks show parameters for all of these included packages:
image
however, as they are just pinned to latest, they just add complexity to the results without providing any new options (they are all [none])

In the latest benchmark run, I dynamically added the two shown options for numpy using set_asv_dependencies; these come from the workflow inputs, set by the user when requesting a new job run

"packaging": [],
"scipy": [],
"matplotlib": []
},
Copy link
Contributor

Choose a reason for hiding this comment

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

We need them to build the wheel in the benchmarks. I don't think we will be able to run the asv benchmark without the matrix.

Copy link
Member

Choose a reason for hiding this comment

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

@MridulS , are you happy with the PR now? Would this work with the PR benchmark you have set up at https://github.com/astropy/astropy/blob/main/.github/workflows/ci_benchmark.yml ? Hope you can advise. Thanks!

@MridulS
Copy link
Contributor

MridulS commented Apr 13, 2024

We can indeed think about adding pins to the dependencies to make the benchmarks even more precise but I'm not sure if we can just get rid of installing them.

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Apr 15, 2024

We need them to build the wheel in the benchmarks. I don't think we will be able to run the asv benchmark without the matrix.

We can indeed think about adding pins to the dependencies to make the benchmarks even more precise but I'm not sure if we can just get rid of installing them.

I removed build, Cython, and packaging, and moved the runtime dependencies from matrix into install_command in 2c57d6e, so that the build command reverts to pip install . and only matplotlib and scipy are installed at runtime:

"install_command": [
"pip install . matplotlib scipy"
],

I ran asv run --quick --dry-run on that on my local machine and it completed successfully. Next I'll try running it in the bench runners against this commit

"build_command": [
"python -m build --wheel -o {build_cache_dir} {build_dir}"
"install_command": [
"pip install . matplotlib scipy"
Copy link
Member

Choose a reason for hiding this comment

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

What if someone wants to run with different versions of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they'd have to manually include those versions in the matrix.reqs in their asv.conf.json; in the case of the bench repository, I added an input to the workflow dispatch that automatically inserts version specifications into the matrix, so in that case the user would specify something like "matplotlib==3.5.1" "matplotlib==3.5.0" in the popup after clicking Run Workflow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

Should this be documented in the README?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yes! Good catch, I'll add that

README.rst Outdated
installed dependency or dependencies, add the ``matrix.req`` entry to
``asv.conf.json``, including the versions you wish to run benchmarks against:

.. code-block:: diff
Copy link
Member

Choose a reason for hiding this comment

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

I think there is a formatting error somewhere here. It is not rendering on "View File".

Copy link
Contributor Author

@zacharyburnett zacharyburnett Apr 15, 2024

Choose a reason for hiding this comment

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

yes I forgot to add a newline after .. code-block:: diff 😅

I ran rst2html and it looks correct:
image

README.rst Outdated
installed dependency or dependencies, add the ``matrix.req`` entry to
``asv.conf.json``, including the version(s) you wish to run benchmarks against:

.. code-block:: diff
Copy link
Member

@pllim pllim Apr 15, 2024

Choose a reason for hiding this comment

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

I don't think diff produces the correct rendering.

(suggestion retracted)

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, nvm, you are trying to show a diff ... But still it is all underlined in preview.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

README.rst Outdated
},
...
}

Copy link
Member

Choose a reason for hiding this comment

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

This works in Markdown but not sure about RST. 🤷‍♀️

--- a/asv.conf.json
+++ b/asv.conf.json
@@ -6,6 +6,17 @@
   "install_command": [
     "pip install . matplotlib scipy"
   ],
+  "matrix": {
+    "req": {
+      "matplotlib": [
+        "3.5.1"
+      ],
+    "numpy": [
+      "1.26.0",
+      "2.0.0rc1"
+      ]
+    }
+  },
   "branches": ["main"],
   "show_commit_url": "http://github.com/astropy/astropy/commit/",
   "pythons": ["3.11"],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think GitHub's rendering of RST is a bit wonky (maybe they want us to use markdown instead). We could change the filetype to markdown, possibly

Copy link
Member

Choose a reason for hiding this comment

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

Would you mind doing that here? It is technically out of scope so you can say no. I just noticed that the Python formatting is also off. 🤷‍♀️

Copy link
Contributor Author

@zacharyburnett zacharyburnett Apr 15, 2024

Choose a reason for hiding this comment

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

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Just minor comments from me. Thanks for the README updates. Formatting definitely is better in markdown. Would still like to hear back from Mridul first if possible.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
"packaging": [],
"scipy": [],
"matplotlib": []
},
Copy link
Member

Choose a reason for hiding this comment

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

@MridulS , are you happy with the PR now? Would this work with the PR benchmark you have set up at https://github.com/astropy/astropy/blob/main/.github/workflows/ci_benchmark.yml ? Hope you can advise. Thanks!

zacharyburnett and others added 2 commits April 16, 2024 09:28
Co-authored-by: P. L. Lim <[email protected]>
Co-authored-by: P. L. Lim <[email protected]>
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Given the doc clarification, I will approve but we should wait for Mridul to have another look?

@pllim
Copy link
Member

pllim commented Apr 16, 2024

Actually @zacharyburnett , would you mind opening an experimental PR at astropy to modify https://github.com/astropy/astropy/blob/main/.github/workflows/ci_benchmark.yml to use benchmark from this branch? You can ping me on that PR and I can put a benchmark label on it, to make sure this works. Thanks!

@zacharyburnett
Copy link
Contributor Author

Actually @zacharyburnett , would you mind opening an experimental PR at astropy to modify https://github.com/astropy/astropy/blob/main/.github/workflows/ci_benchmark.yml to use benchmark from this branch? You can ping me on that PR and I can put a benchmark label on it, to make sure this works. Thanks!

sure thing! Here: astropy/astropy#16296

@pllim
Copy link
Member

pllim commented Apr 18, 2024

Conclusion: astropy/astropy#16296 (comment)

Should we merge? 😸

@zacharyburnett
Copy link
Contributor Author

sounds good! Let's merge

Copy link
Contributor

@MridulS MridulS left a comment

Choose a reason for hiding this comment

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

This looks good to me, now I don't remember why I wanted to build the wheel in the first place.

@pllim pllim merged commit ba0d89f into astropy:main Apr 18, 2024
@pllim
Copy link
Member

pllim commented Apr 18, 2024

Thanks, all!

@zacharyburnett zacharyburnett deleted the fix/asv_conf branch April 18, 2024 18:17
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.

3 participants