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

Install the Catalyst CLI into user Python environment #1434

Open
wants to merge 4 commits into
base: v0.10.0-rc
Choose a base branch
from

Conversation

dime10
Copy link
Contributor

@dime10 dime10 commented Jan 9, 2025

Renames the Catalyst CLI command name from catalyst-cli to catalyst, and also installs the tool in the active Python environment when installing from wheel or from non-editable source.

dime10 added 2 commits January 9, 2025 18:04
Note that the name of the tool is still "Catalyst CLI" (e.g. for use
in text). Also, the CMake target name has been kept at `catalyst-cli`
to avoid unnecessarily modifying build scripts.
@dime10 dime10 added the author:build-wheels Run the wheel building workflows on this Pull Request label Jan 9, 2025
Copy link
Contributor

github-actions bot commented Jan 9, 2025

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md on your branch with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@dime10 dime10 added this to the v0.10.0 milestone Jan 9, 2025
@joeycarter joeycarter added the urgent Mark a pull request as high priority label Jan 10, 2025
@dime10 dime10 removed the urgent Mark a pull request as high priority label Jan 10, 2025
Copy link
Contributor

@erick-xanadu erick-xanadu 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! However, didn't the macOS arm wheel test failed? https://github.com/PennyLaneAI/catalyst/actions/runs/12700501137 I am not sure why yet.

@dime10
Copy link
Contributor Author

dime10 commented Jan 10, 2025

This looks good! However, didn't the macOS arm wheel test failed? https://github.com/PennyLaneAI/catalyst/actions/runs/12700501137 I am not sure why yet.

Looks like they are already present on the RC: https://github.com/PennyLaneAI/catalyst/actions/runs/12712591456/job/35439374566

Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Just a few fly-by observations

@@ -205,7 +205,7 @@ wheel:
cp $(COPY_FLAGS) $(DIALECTS_BUILD_DIR)/python_packages/quantum/mlir_quantum/dialects/*$${file}* $(MK_DIR)/frontend/mlir_quantum/dialects ; \
done
mkdir -p $(MK_DIR)/frontend/catalyst/bin
cp $(COPY_FLAGS) $(DIALECTS_BUILD_DIR)/bin/catalyst-cli $(MK_DIR)/frontend/catalyst/bin
cp $(COPY_FLAGS) $(DIALECTS_BUILD_DIR)/bin/catalyst $(MK_DIR)/frontend/catalyst/bin
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 possible with CMake's installation support, rather than explicit copies?

@@ -319,6 +319,15 @@ def run(self):
# - `ops`: Path to the compiler operations module.
# - `qjit`: Path to the JIT compiler decorator provided by the compiler.

# Install the `catalyst` binary into the user's Python environment so it is accessible on the PATH.
# Does not work with editable installs. Requires the Catalyst mlir module to be built.
if os.path.exists("frontend/catalyst/bin/catalyst"):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if os.path.exists("frontend/catalyst/bin/catalyst"):
if pathlib.Path("frontend", "catalyst", "bin", "catalyst").exists()

I'd recommend using the system agnostic pathlib with this and below.

@@ -338,6 +347,9 @@ def run(self):
),
package_dir={"": "frontend"},
include_package_data=True,
data_files=[
Copy link
Member

Choose a reason for hiding this comment

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

Note that use of data_files itself is discouraged https://setuptools.pypa.io/en/latest/references/keywords.html
Probably fine for now, but likely good to plan to handle this in the recommended way instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author:build-wheels Run the wheel building workflows on this Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants