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

Support for disabling functions with feature flags #333

Closed
eitsupi opened this issue Dec 29, 2024 · 13 comments · Fixed by #334
Closed

Support for disabling functions with feature flags #333

eitsupi opened this issue Dec 29, 2024 · 13 comments · Fixed by #334

Comments

@eitsupi
Copy link
Contributor

eitsupi commented Dec 29, 2024

We may want to disable a function that does not work on a particular platform by using a feature flag.
For example, the following functions in Python Polars do not work on WASM and will be disabled in the build for WASM.

#[pymethods]
impl PyExpr {
    #[cfg(feature = "extract_jsonpath")]
    fn str_json_path_match(&self, pat: Self) -> Self {
        self.inner.clone().str().json_path_match(pat.inner).into()
    }
}

https://github.com/pola-rs/polars/blob/f5f4cb55e98c60f723036b932796d972104b5f93/crates/polars-python/src/expr/string.rs#L243-L246

Compiling the following code, which does the same thing using savvy, yields an error.

#[savvy]
impl PlRExpr {
    #[cfg(not(target_arch = "wasm32"))]
    fn str_json_path_match(&self, pat: &PlRExpr) -> Result<Self> {
        Ok(self
            .inner
            .clone()
            .str()
            .json_path_match(pat.inner.clone())
            .into())
    }
}

https://github.com/eitsupi/neo-r-polars/blob/14b234c300779ef4e7d0612bbfe9aaac03d8502e/src/rust/src/expr/string.rs#L114-L123

error[E0425]: cannot find function `savvy_PlRExpr_str_json_path_match_inner` in this scope
 --> src/expr/string.rs:5:6
  |
4 | #[savvy]
  | -------- similarly named function `savvy_PlRExpr_str_json_path_match__ffi` defined here
5 | impl PlRExpr {
  |      ^^^^^^^ help: a function with a similar name exists: `savvy_PlRExpr_str_json_path_match__ffi`

https://github.com/eitsupi/neo-r-polars/actions/runs/12536892771/job/34960399544

Using the following approach, Rust compiles successfully, but the R package build fails because multiple functions with the same name are generated on the C file generated by the savvy-cli update command.

#[cfg(not(target_arch = "wasm32"))]
#[savvy]
impl PlRExpr {
    fn str_json_path_match(&self, pat: &PlRExpr) -> Result<Self> {
        Ok(self
            .inner
            .clone()
            .str()
            .json_path_match(pat.inner.clone())
            .into())
    }
}

#[cfg(target_arch = "wasm32")]
#[savvy]
impl PlRExpr {
    #[allow(unused_variables)]
    fn str_json_path_match(&self, pat: &PlRExpr) -> Result<Self> {
        Err(RPolarsErr::Other(format!("Not supported in WASM")).into())
    }
}
@yutannihilation
Copy link
Owner

Unfortunately, I don't think this will be possible. Handling feature flags properly is a too heavy job for static analysis.

On the other hand, I think this should be improved. Maybe just ignore duplicated one if the signatures match, and raise an error if the signatures don't match?

but the R package build fails because multiple functions with the same name are generated on the C file

@eitsupi
Copy link
Contributor Author

eitsupi commented Jan 3, 2025

Thanks for your reply!

On the other hand, I think this should be improved. Maybe just ignore duplicated one if the signatures match, and raise an error if the signatures don't match?

I think that makes sense.

@yutannihilation
Copy link
Owner

@eitsupi
The binary of the fixed version is available here. Could you test when you have time? No hurry at all!

https://github.com/yutannihilation/savvy/releases/tag/v0.8.3-rc.1

@eitsupi
Copy link
Contributor Author

eitsupi commented Jan 8, 2025

Thanks for the update!
I have confirmed that the following pattern works:

#[cfg(not(target_arch = "wasm32"))]
#[savvy]
impl PlRExpr {
    fn str_json_path_match(&self, pat: &PlRExpr) -> Result<Self> {
        Ok(self
            .inner
            .clone()
            .str()
            .json_path_match(pat.inner.clone())
            .into())
    }
}

#[cfg(target_arch = "wasm32")]
#[savvy]
impl PlRExpr {
    #[allow(unused_variables)]
    fn str_json_path_match(&self, pat: &PlRExpr) -> Result<Self> {
        Err(RPolarsErr::Other(format!("Not supported in WASM")).into())
    }
}

I am wondering if the following pattern are also supported (I will have time to try within the next few days)

#[savvy]
impl PlRExpr {
    #[cfg(not(target_arch = "wasm32"))]
    fn str_json_path_match(&self, pat: &PlRExpr) -> Result<Self> {
        Ok(self
            .inner
            .clone()
            .str()
            .json_path_match(pat.inner.clone())
            .into())
    }

    #[cfg(target_arch = "wasm32")]
    #[allow(unused_variables)]
    fn str_json_path_match(&self, pat: &PlRExpr) -> Result<Self> {
        Err(RPolarsErr::Other(format!("Not supported in WASM")).into())
    }
}

@eitsupi
Copy link
Contributor Author

eitsupi commented Jan 9, 2025

I am wondering if the following pattern are also supported (I will have time to try within the next few days)

#[savvy]
impl PlRExpr {
    #[cfg(not(target_arch = "wasm32"))]
    fn str_json_path_match(&self, pat: &PlRExpr) -> Result<Self> {
        Ok(self
            .inner
            .clone()
            .str()
            .json_path_match(pat.inner.clone())
            .into())
    }

    #[cfg(target_arch = "wasm32")]
    #[allow(unused_variables)]
    fn str_json_path_match(&self, pat: &PlRExpr) -> Result<Self> {
        Err(RPolarsErr::Other(format!("Not supported in WASM")).into())
    }
}

This pattern does not seems supported.

@yutannihilation
Copy link
Owner

Thanks for testing. Let me check.

@yutannihilation
Copy link
Owner

I can't reproduce the problem. What error do you see when you run savvy-cli update?

@yutannihilation
Copy link
Owner

Ah, sorry, I hit the problem. Please forget my comment above.

error[E0428]: the name `savvy_StructWithConfig_new__ffi` is defined multiple times
  --> /Users/runner/work/savvy/savvy/R-package/src/rust/src/multiple_defs.rs:20:1
   |
20 | #[savvy]
   | ^^^^^^^^ `savvy_StructWithConfig_new__ffi` redefined here
   |
   = note: `savvy_StructWithConfig_new__ffi` must be defined only once in the value namespace of this module
   = note: this error originates in the attribute macro `savvy` (in Nightly builds, run with -Z macro-backtrace for more info)

@yutannihilation
Copy link
Owner

yutannihilation commented Jan 10, 2025

It turned out that the problem is on #[savvy] macro. Should be fixed now, but you need to specify the RC version in your Cargo.toml to try this.

https://crates.io/crates/savvy/0.8.3-rc.2

@eitsupi
Copy link
Contributor Author

eitsupi commented Jan 10, 2025

Thanks for the update!
But perhaps associated functions still seem to fail?

   error[E0428]: the name `savvy_PlRLazyFrame_new_from_parquet__ffi` is defined multiple times
     --> src/lazyframe/general.rs:12:1
      |
   12 | #[savvy]
      | ^^^^^^^^ `savvy_PlRLazyFrame_new_from_parquet__ffi` redefined here
      |
      = note: `savvy_PlRLazyFrame_new_from_parquet__ffi` must be defined only once in the value namespace of this module
      = note: this error originates in the attribute macro `savvy` (in Nightly builds, run with -Z macro-backtrace for more info)
   
   error[E0428]: the name `savvy_PlRLazyFrame_new_from_ndjson__ffi` is defined multiple times
     --> src/lazyframe/general.rs:12:1
      |
   12 | #[savvy]
      | ^^^^^^^^ `savvy_PlRLazyFrame_new_from_ndjson__ffi` redefined here
      |
      = note: `savvy_PlRLazyFrame_new_from_ndjson__ffi` must be defined only once in the value namespace of this module
      = note: this error originates in the attribute macro `savvy` (in Nightly builds, run with -Z macro-backtrace for more info)
   
✔ Setting active project to "/workspaces/neo-r-polars".
   For more information about this error, try `rustc --explain E0428`.
   error: could not compile `r-polars` (lib) due to 2 previous errors

from eitsupi/neo-r-polars@00fdcad

@yutannihilation
Copy link
Owner

This version should work!

https://crates.io/crates/savvy/0.8.3-rc.4

@eitsupi
Copy link
Contributor Author

eitsupi commented Jan 11, 2025

Confirmed in eitsupi/neo-r-polars#50, thanks!

@yutannihilation
Copy link
Owner

Great. Let's merge and release then!

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 a pull request may close this issue.

2 participants