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

fix: remove evals from model parsing #2379

Merged

Conversation

ven-k
Copy link
Member

@ven-k ven-k commented Dec 11, 2023

Closes #2377

  • Along with MTKModels with component arrays, the ones with icons of Expr type had this issue.
  • This PR ensures that modules with either of those can be precompiled.

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the ScioML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

function parse_icon!(icon, dict, body::Expr)
parse_icon!(icon, dict, eval(body))
function parse_icon!(body::Union{Expr, Symbol}, dict, icon, mod)
parse_icon!(Core.eval(mod, body), dict, icon, mod)
Copy link
Member

@ChrisRackauckas ChrisRackauckas Dec 13, 2023

Choose a reason for hiding this comment

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

Suggested change
parse_icon!(Core.eval(mod, body), dict, icon, mod)
parse_icon!(getfield(mod, :body), dict, icon, mod)

? Example of what you're evaling

Copy link
Member Author

Choose a reason for hiding this comment

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

This

@mtkmodel ModelWithExprIcon begin
    @icon read(abspath(@__DIR__, "..", "icons", "ground.svg"), String)
    # or @icon ground_logo 
end

from tests - here is an example.

In general,
anything with

@mtkmodel A begin
    @icon begin
        ...
    end
end

The body is an expression that should be evaluated within the defining module mod; in our test example that is ModelParsingPrecompile.
getfield can't be used as body is an expression in the module and not a function or a variable.

Copy link
Member

Choose a reason for hiding this comment

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

Simplify that to just the string as a path to an SVG?

Copy link
Member Author

@ven-k ven-k Dec 13, 2023

Choose a reason for hiding this comment

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

I checked; if @icon abspath() is used instead of plane string, that would need this too.

Core.eval doesn't suffer with the same issue as Base.eval; is there any performance issue with it?

In that case we can force to assign abspath() value to a var; and rewrite this only for Symbol (and drop Expr)

parse_icon!(body::Symbol, dict, icon, mod) = parse_icon!(getfield(mod, body), dict, icon, mod)

Copy link
Member Author

@ven-k ven-k Dec 14, 2023

Choose a reason for hiding this comment

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

I've dropped the Core.eval and Expr dispatch from the @icon parser.

Resolved it here.

Copy link
Member Author

@ven-k ven-k Dec 14, 2023

Choose a reason for hiding this comment

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

Doc doesn't need to be updated as it uses strings to showcase all 3 ways (URI, inlined SVG, paths).

@ven-k ven-k force-pushed the vkb/fix-precompilation-of-mtkmodel branch 2 times, most recently from 409a05d to 1a556f2 Compare December 14, 2023 12:43
@ChrisRackauckas
Copy link
Member

Rebase?

@ven-k
Copy link
Member Author

ven-k commented Dec 14, 2023

Already done 👍

- Ensure that modules consisting MTKModels with component arrays and icons of
`Expr` type and `unit` metadata can be precompiled.
@ven-k ven-k force-pushed the vkb/fix-precompilation-of-mtkmodel branch from b543c45 to e60c214 Compare December 21, 2023 16:43
@ChrisRackauckas ChrisRackauckas merged commit ce5dbbb into SciML:master Dec 22, 2023
13 of 22 checks passed
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.

Broadcast inside of @mtkmodel macro causes precompilation failures when put into a package
2 participants