Completed python stub generation (with caveats)

CHANGELOG.md

@@ -2,10 +2,12 @@
 
 Changes in each release are listed below.
 
-## 1.7.0 18-Oct-2024
+## 1.7.0 23-Oct-2024
 
 * Update fitsio to 0.21 and fitsio-sys to 0.5.
 * Removed Rust Report Card from README status badges. Looks like this service is abandonded.
+* Added Python .pyi stub generation to provide mwalib Python users with type and docstring information. The mwalib.pyi should get baked into the python wheels released to github and Pypi. See `bin/README.md` for caveats and more details.
+* Added CI to test compilation against cfitsio 3.x and 4.x when not using the `cfitsio-static` feature.
 
 ## 1.6.0 18-Oct-2024
 

Cargo.toml

@@ -14,12 +14,20 @@ keywords = ["radioastronomy", "mwa", "astronomy"]
 categories = ["science","parsing"]
 exclude = ["test_files/*", "tools/*",".github/*"]
 
+[[bin]]
+name = "stub_gen"
+path = "bin/stub_gen.rs"
+doc = false
+required-features = ["python"]
+
 # Make a rust library, as well as static and C-compatible dynamic libraries
 # available as "libmwalib.a" and "libmwalib.so".
 [lib]
 crate-type = ["rlib", "staticlib", "cdylib"]
 
 [features]
+# default
+default = ["cfitsio-static", "examples"]
 # Compile cfitsio from source and link it statically.
 cfitsio-static = ["fitsio-sys/fitsio-src"]
 # Enable optional features needed by examples.
@@ -44,10 +52,10 @@ thiserror = "~1.0"
 anyhow = { version = "~1.0", optional = true }
 env_logger = { version = "~0.11", optional = true }
 
-# 'python' feature
+# "python" feature
 ndarray = { version = "~0.16", optional = true }
 numpy = { version = "~0.22", optional = true }
-pyo3 = { version = "~0.22", features = ["chrono", "extension-module"], optional = true }
+pyo3 = { version = "~0.22", features = ["chrono", "extension-module", "macros"], optional = true }
 pyo3-stub-gen = { version = "~0.6", optional = true }
 pyo3-stub-gen-derive = { version = "~0.6", optional = true }
 
@@ -89,9 +97,4 @@ required-features = ["examples"]
 
 [[example]]
 name = "mwalib-sum-first-fine-channel-gpubox-hdus"
-required-features = ["examples"]
-
-[[bin]]
-name = "stub_gen"
-path = "bin/stub_gen.rs"
-doc = false
+required-features = ["examples"]

bin/README.md

@@ -0,0 +1,20 @@
+# mwalib python pyi stub generation
+
+## How it works
+
+1. Build mwalib using: `cargo build --all-features`
+2. Run the stub generator: `target/debug/stub_gen`
+3. At this point you can view the `mwalib.pyi` file and see what Python stubs were generated.
+4. To test, build a python wheel to test with: `maturin build --all-features --out dist`.
+5. In a freah Python environment install the wheel: `pip install dist/mwalib-1.7.0-cp313-cp313-manylinux_2_34_x86_64.whl`.
+6. Open a python file in your IDE and hopefully you have some type info and some doc strings.
+
+## Caveats
+
+* Due to [this issue](https://github.com/Jij-Inc/pyo3-stub-gen/issues/93) and [this issue](https://github.com/PyO3/pyo3/issues/780), the codem as of mwalib 1.7.0 does NOT produce a full mwalib.pyi file, due to the fact that the stub generation requires the `python` feature and to get all the struct members to appear in the stub you need to decorate each member with `#[pyo3(get,set)]` but this decorator does not work with the `#[cfg_attr(feature = "python", pyo3(get,set))]` syntax needed to allow mwalib to be compiled without the `python` feature! So to get past this, I have removed the `#[cfg_attr(feature = "python", pyo3(get,set))]` syntax from all struct members and changed `#[cfg_attr(feature = "python", pyo3::pyclass]` on each struct to `#[cfg_attr(feature = "python", pyo3::pyclass(get_all, set_all))]` which will still create the python bindings but none of the struct members will be emitted when generating the stub file!
+
+* Docstrings for `#[new]` methods on structs/classes do not get generated.
+
+* `__enter__` method for a class gets the wrong generated stub so I have to override it (see below).
+
+* Some other manual fixes can be seen in `bin/stubgen.rs`. Hacky but we live in an imperfect world!

bin/stub_gen.rs

@@ -1,16 +1,41 @@
 extern crate mwalib;
+
+#[cfg(feature = "python")]
 use std::env;
+#[cfg(feature = "python")]
 use std::fs::File;
+#[cfg(feature = "python")]
 use std::io::{Read, Write};
+#[cfg(feature = "python")]
+use std::path::Path;
+
+#[cfg(test)]
+#[cfg(feature = "python")]
+use tempdir::TempDir;
 
+#[cfg(feature = "python")]
 fn main() -> anyhow::Result<()> {
     env_logger::Builder::from_env(env_logger::Env::default().filter_or("RUST_LOG", "info")).init();
 
+    generate_stubs()?;
+
+    fix_stubs()?;
+
+    anyhow::Ok(())
+}
+
+#[cfg(feature = "python")]
+fn generate_stubs() -> anyhow::Result<()> {
     // Generating the stub requires the below env variable to be set for some reason?
     env::set_var("CARGO_MANIFEST_DIR", env::current_dir()?);
     let stub = mwalib::python::stub_info()?;
     stub.generate()?;
 
+    Ok(())
+}
+
+#[cfg(feature = "python")]
+fn fix_stubs() -> anyhow::Result<()> {
     // After the stub is generated, we have some "manual" fixes to do
     let stubfile = String::from("mwalib.pyi");
 
@@ -36,19 +61,11 @@ fn main() -> anyhow::Result<()> {
     )?;
 
     // Replace the constructors as pyo3_stub_gen seems to ignore the text_signature
-    replace_stub(
-        &stubfile,
-        "def __new__(cls,metafits_filename,mwa_version = ...): ...",
-        "def __new__(cls, metafits_filename: str, mwa_version: typing.Optional[MWAVersion]=None)->MetafitsContext:\n        ...\n",
-    )?;
+    replace_stub(&stubfile,"def __new__(cls,metafits_filename,mwa_version = ...): ...","def __new__(cls, metafits_filename: str, mwa_version: typing.Optional[MWAVersion]=None)->MetafitsContext:\n        ...\n",)?;
 
     replace_stub(&stubfile, "def __new__(cls,metafits_filename,gpubox_filenames): ...", "def __new__(cls, metafits_filename: str, gpubox_filenames: list[str])->CorrelatorContext:\n        ...\n")?;
 
-    replace_stub(
-        &stubfile,
-        "def __new__(cls,metafits_filename,voltage_filenames): ...",
-        "def __new__(cls, metafits_filename: str, voltage_filenames: list[str])->VoltageContext:\n        ...\n",
-    )?;
+    replace_stub(&stubfile,"def __new__(cls,metafits_filename,voltage_filenames): ...","def __new__(cls, metafits_filename: str, voltage_filenames: list[str])->VoltageContext:\n        ...\n",)?;
 
     replace_stub(
         &stubfile,
@@ -68,7 +85,7 @@ fn main() -> anyhow::Result<()> {
         "def __enter__(self) -> VoltageContext:",
     )?;
 
-    anyhow::Ok(())
+    Ok(())
 }
 
 /// Inserts new items in the stubfile for when pyo3 cant do it
@@ -81,7 +98,7 @@ fn main() -> anyhow::Result<()> {
 ///
 /// # Arguments
 ///
-/// * `stubfile` - filename of the stubfile to edit
+/// * `stubfile` - `Path` representing filename of the stubfile to edit
 ///
 /// * `string_to_find` - string to find, so we know where in the file to make the insert
 ///
@@ -93,13 +110,14 @@ fn main() -> anyhow::Result<()> {
 /// * Result Ok if stub file was modified successfully.
 ///
 ///
-fn insert_stub_below(
-    stubfile: &str,
+#[cfg(feature = "python")]
+fn insert_stub_below<P: AsRef<Path>>(
+    stubfile: P,
     string_to_find: &str,
     string_to_add_below: &str,
 ) -> anyhow::Result<()> {
     // Open and read the file entirely
-    let mut src = File::open(stubfile)?;
+    let mut src = File::open(stubfile.as_ref())?;
     let mut data = String::new();
     src.read_to_string(&mut data)?;
     drop(src); // Close the file early
@@ -110,7 +128,7 @@ fn insert_stub_below(
     let new_data = data.replace(string_to_find, &new_string);
 
     // Recreate the file and dump the processed contents to it
-    let mut dst = File::create(stubfile)?;
+    let mut dst = File::create(stubfile.as_ref())?;
     dst.write_all(new_data.as_bytes())?;
 
     anyhow::Ok(())
@@ -126,7 +144,7 @@ fn insert_stub_below(
 ///
 /// # Arguments
 ///
-/// * `stubfile` - filename of the stubfile to edit
+/// * `stubfile` - `Path` representing filename of the stubfile to edit
 ///
 /// * `string_to_find` - string to find (which will be replaced)
 ///
@@ -138,13 +156,14 @@ fn insert_stub_below(
 /// * Result Ok if stub file was modified successfully.
 ///
 ///
-fn replace_stub(
-    stubfile: &str,
+#[cfg(feature = "python")]
+fn replace_stub<P: AsRef<Path>>(
+    stubfile: P,
     string_to_find: &str,
     string_to_replace: &str,
 ) -> anyhow::Result<()> {
     // Open and read the file entirely
-    let mut src = File::open(stubfile)?;
+    let mut src = File::open(stubfile.as_ref())?;
     let mut data = String::new();
     src.read_to_string(&mut data)?;
     drop(src); // Close the file early
@@ -153,49 +172,62 @@ fn replace_stub(
     let new_data = data.replace(string_to_find, string_to_replace);
 
     // Recreate the file and dump the processed contents to it
-    let mut dst = File::create(stubfile)?;
+    let mut dst = File::create(stubfile.as_ref())?;
     dst.write_all(new_data.as_bytes())?;
 
     anyhow::Ok(())
 }
 
 #[test]
+#[cfg(feature = "python")]
 fn test_insert_stub_below() {
+    // Create ephemeral temp directory which will be deleted at end of test
+    let dir =
+        TempDir::new("test_insert_stub_below").expect("Cannot create temp directory for test");
+
     // Create a test file
-    let text_filename: String = "test.pyi".to_string();
+    let text_filename = dir.path().join("test.pyi");
 
-    let mut test_file =
-        File::create(&text_filename).unwrap_or_else(|_| panic!("Could not open {}", text_filename));
+    let mut test_file = File::create(&text_filename)
+        .unwrap_or_else(|_| panic!("Could not open {}", text_filename.to_str().unwrap()));
     // Write some lines
     let content = "    Hello\n    World\n    1234";
     let _ = test_file.write_all(content.as_bytes());
 
     // Now run the add_stub command
     insert_stub_below(&text_filename, "    World\n", "    added_string\n")
-        .unwrap_or_else(|_| panic!("Could not add_stub to {}", text_filename));
+        .unwrap_or_else(|_| panic!("Could not add_stub to {}", text_filename.to_str().unwrap()));
 
     // Now reread the file
-    let test_file =
-        File::open(&text_filename).unwrap_or_else(|_| panic!("Could not open {}", text_filename));
+    let test_file = File::open(&text_filename)
+        .unwrap_or_else(|_| panic!("Could not open {}", text_filename.to_str().unwrap()));
     let mut lines = Vec::new();
 
     for line in std::io::read_to_string(test_file).unwrap().lines() {
         lines.push(line.to_string())
     }
 
+    // Remove our temp dir
+    dir.close().expect("Failed to close temp directory");
+
     assert_eq!(lines[0], "    Hello");
     assert_eq!(lines[1], "    World");
     assert_eq!(lines[2], "    added_string");
     assert_eq!(lines[3], "    1234");
 }
 
 #[test]
+#[cfg(feature = "python")]
 fn test_replace_stub() {
+    // Create ephemeral temp directory which will be deleted at end of test
+    let dir =
+        TempDir::new("mwalib_test_replace_stub").expect("Cannot create temp directory for test");
+
     // Create a test file
-    let text_filename: String = "test.pyi".to_string();
+    let text_filename = dir.path().join("test.pyi");
 
-    let mut test_file =
-        File::create(&text_filename).unwrap_or_else(|_| panic!("Could not open {}", text_filename));
+    let mut test_file = File::create(&text_filename)
+        .unwrap_or_else(|_| panic!("Could not open {}", text_filename.to_str().unwrap()));
     // Write some lines
     let content = "    Hello\n    World\n    1234";
     let _ = test_file.write_all(content.as_bytes());
@@ -206,17 +238,20 @@ fn test_replace_stub() {
         "    World\n",
         "    This is the replaced string\n",
     )
-    .unwrap_or_else(|_| panic!("Could not add_stub to {}", text_filename));
+    .unwrap_or_else(|_| panic!("Could not add_stub to {}", text_filename.to_str().unwrap()));
 
     // Now reread the file
-    let test_file =
-        File::open(&text_filename).unwrap_or_else(|_| panic!("Could not open {}", text_filename));
+    let test_file = File::open(&text_filename)
+        .unwrap_or_else(|_| panic!("Could not open {}", text_filename.to_str().unwrap()));
     let mut lines = Vec::new();
 
     for line in std::io::read_to_string(test_file).unwrap().lines() {
         lines.push(line.to_string())
     }
 
+    // Remove our temp dir
+    dir.close().expect("Failed to close temp directory");
+
     assert_eq!(lines[0], "    Hello");
     assert_eq!(lines[1], "    This is the replaced string");
     assert_eq!(lines[2], "    1234");