[docs] tick off macro refactoring from roadmap #3629

odow · 2023-12-14T20:42:33Z

I have made a whole suite of changes recently:

Looking at a diff isn't too useful, but here was the code before I started:

https://github.com/jump-dev/JuMP.jl/blob/v1.17.0/src/macros.jl

And here it is now:

https://github.com/jump-dev/JuMP.jl/blob/master/src/macros.jl
https://github.com/jump-dev/JuMP.jl/tree/master/src/macros
https://github.com/jump-dev/JuMP.jl/blob/master/src/Containers/macro.jl

I'm particularly pleased with the now very readable:

JuMP.jl/src/macros/@expression.jl

Lines 66 to 98 in 65d12dc

    
           macro expression(input_args...) 
        
               error_fn = Containers.build_error_fn(:expression, input_args, __source__) 
        
               args, kwargs = Containers.parse_macro_arguments( 
        
                   error_fn, 
        
                   input_args; 
        
                   num_positional_args = 2:3, 
        
                   valid_kwargs = [:container], 
        
               ) 
        
               if Meta.isexpr(args[2], :block) 
        
                   error_fn("Invalid syntax. Did you mean to use `@expressions`?") 
        
               end 
        
               name_expr = length(args) == 3 ? args[2] : nothing 
        
               name, index_vars, indices = Containers.parse_ref_sets( 
        
                   error_fn, 
        
                   name_expr; 
        
                   invalid_index_variables = [args[1]], 
        
               ) 
        
               model = esc(args[1]) 
        
               expr, build_code = _rewrite_expression(args[end]) 
        
               code = quote 
        
                   $build_code 
        
                   # Don't leak a `_MA.Zero` if the expression is an empty summation, or 
        
                   # other structure that returns `_MA.Zero()`. 
        
                   _replace_zero($model, $expr) 
        
               end 
        
               return _finalize_macro( 
        
                   model, 
        
                   Containers.container_code(index_vars, indices, code, kwargs), 
        
                   __source__; 
        
                   register_name = name, 
        
                   wrap_let = true, 
        
               ) 
        
           end

when it was

JuMP.jl/src/macros.jl

Lines 1923 to 1975 in 419c334

    
           macro expression(args...) 
        
               _error(str...) = _macro_error(:expression, args, __source__, str...) 
        
               args, kw_args, requested_container = Containers._extract_kw_args(args) 
        
               if length(args) == 3 
        
                   m = esc(args[1]) 
        
                   c = args[2] 
        
                   x = args[3] 
        
               elseif length(args) == 2 
        
                   m = esc(args[1]) 
        
                   c = gensym() 
        
                   x = args[2] 
        
               else 
        
                   _error("needs at least two arguments.") 
        
               end 
        
               length(kw_args) == 0 || _error("unrecognized keyword argument") 
        
               if isexpr(args[2], :block) 
        
                   _error("Invalid syntax. Did you mean to use `@expressions`?") 
        
               end 
        
               anonvar = isexpr(c, :vect) || isexpr(c, :vcat) || length(args) == 2 
        
               variable = gensym() 
        
               idxvars, indices = Containers.build_ref_sets(_error, c) 
        
               if args[1] in idxvars 
        
                   _error( 
        
                       "Index $(args[1]) is the same symbol as the model. Use a " * 
        
                       "different name for the index.", 
        
                   ) 
        
               end 
        
               expr_var, build_code = _rewrite_expression(x) 
        
               code = quote 
        
                   $build_code 
        
                   # Don't leak a `_MA.Zero` if the expression is an empty summation, or 
        
                   # other structure that returns `_MA.Zero()`. 
        
                   _replace_zero($m, $expr_var) 
        
               end 
        
               code = 
        
                   Containers.container_code(idxvars, indices, code, requested_container) 
        
               # Wrap the entire code block in a let statement to make the model act as 
        
               # a type stable local variable. 
        
               code = _wrap_let(m, code) 
        
               # don't do anything with the model, but check that it's valid anyway 
        
               if anonvar 
        
                   macro_code = code 
        
               else 
        
                   macro_code = _macro_assign_and_return( 
        
                       code, 
        
                       variable, 
        
                       Containers._get_name(c); 
        
                       model_for_registering = m, 
        
                   ) 
        
               end 
        
               return _finalize_macro(m, macro_code, __source__) 
        
           end

For extensions, @pulsipher has been trying this out in InfiniteOpt: infiniteopt/InfiniteOpt.jl#334, and I think it's a big win.

Now is the chance to take a read through and open up any bike shedding, etc.

codecov · 2023-12-14T20:55:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (817dc13) 98.27% compared to head (fdd4b6e) 98.27%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3629   +/-   ##
=======================================
  Coverage   98.27%   98.27%           
=======================================
  Files          43       43           
  Lines        5636     5636           
=======================================
  Hits         5539     5539           
  Misses         97       97

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

odow · 2023-12-14T21:39:45Z

TODOs

odow · 2023-12-15T00:44:53Z

So Plasmo uses Containers._extract_kw_args. I've added it back for now. But it's a good example of why a proper public API is needed.

cc @dlcole3

mlubin · 2023-12-16T19:24:30Z

So clean. How confident are we about the test coverage and not breaking things with this big refactor?

odow · 2023-12-16T19:28:20Z

How confident are we about the test coverage and not breaking things with this big refactor?

I'm fairly confident.

I actually made very minimal changes to how the macros process code and the steps they do it in. it was mainly just refactoring common operations into some now public Containers.xxx functions.

That said, we know that the test coverage is not comprehensive, because #3631 was only picked up by the extension tests. So there are some unknown unknowns, where we don't know what parts we're missing tests for.

The main risk is probably esc scoping bugs. I haven't added new error messages etc.

The extension tests pass now though: https://github.com/jump-dev/JuMP.jl/actions/runs/7216451785 (With the exception of an unrelated printing change in DisjunctiveProgramming)

mlubin · 2023-12-16T19:30:07Z

Do we have good coverage for error messages also?

odow · 2023-12-17T08:03:05Z

Do we have good coverage for error messages also?

We're not quite at 100%. Just a few lines missing, but from a glance they look okay:

https://app.codecov.io/gh/jump-dev/JuMP.jl/blob/master/src%2Fmacros%2F%40constraint.jl#L671
https://app.codecov.io/gh/jump-dev/JuMP.jl/blob/master/src%2Fmacros%2F%40variable.jl#L192
https://app.codecov.io/gh/jump-dev/JuMP.jl/blob/master/src%2Fmacros%2F%40variable.jl#L261

I can open another PR that adds some more tests

mlubin

LGTM but let's have benoit approve the roadmap item completion also

blegat

Looks good to me too, it's nice to have this refactored into the Containers submodule so that the non-optimization specific part is decoupled. Actually, if this submodule was a separate package, I could use it in DynamicPolynomials in the @polyvar macro.

odow · 2023-12-19T20:51:12Z

Actually, if this submodule was a separate package

It's starting to seem like that might be good. But there's no rush.

odow requested review from blegat and mlubin December 14, 2023 20:42

odow mentioned this pull request Dec 14, 2023

Fix escaping of base_name arguments in macros #3631

Merged

odow force-pushed the od/macro-refactor-done branch from cd5b0c3 to 9f4f355 Compare December 15, 2023 00:19

mlubin approved these changes Dec 17, 2023

View reviewed changes

This was referenced Dec 17, 2023

Fix broadcasting f-in-set in constraint macro #3632

Merged

Improve test coverage in src/macros/@variable.jl #3633

Merged

odow added 2 commits December 18, 2023 13:47

[docs] tick off macro refactoring from roadmap

25ac0f3

Revert _extract_kw_args

fdd4b6e

odow force-pushed the od/macro-refactor-done branch from b1b76ac to fdd4b6e Compare December 18, 2023 00:48

blegat approved these changes Dec 19, 2023

View reviewed changes

odow merged commit f5f07b5 into master Dec 19, 2023
11 checks passed

odow deleted the od/macro-refactor-done branch December 19, 2023 20:51

odow mentioned this pull request Jan 5, 2024

@variable macro fails to resolve variables in 1.18.0 #3646

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[docs] tick off macro refactoring from roadmap #3629

[docs] tick off macro refactoring from roadmap #3629

odow commented Dec 14, 2023 •

edited

Loading

codecov bot commented Dec 14, 2023 •

edited

Loading

odow commented Dec 14, 2023 •

edited

Loading

odow commented Dec 15, 2023 •

edited

Loading

mlubin commented Dec 16, 2023

odow commented Dec 16, 2023 •

edited

Loading

mlubin commented Dec 16, 2023

odow commented Dec 17, 2023

mlubin left a comment

blegat left a comment

odow commented Dec 19, 2023

	macro expression(input_args...)
	error_fn = Containers.build_error_fn(:expression, input_args, __source__)
	args, kwargs = Containers.parse_macro_arguments(
	error_fn,
	input_args;
	num_positional_args = 2:3,
	valid_kwargs = [:container],
	)
	if Meta.isexpr(args[2], :block)
	error_fn("Invalid syntax. Did you mean to use `@expressions`?")
	end
	name_expr = length(args) == 3 ? args[2] : nothing
	name, index_vars, indices = Containers.parse_ref_sets(
	error_fn,
	name_expr;
	invalid_index_variables = [args[1]],
	)
	model = esc(args[1])
	expr, build_code = _rewrite_expression(args[end])
	code = quote
	$build_code
	# Don't leak a `_MA.Zero` if the expression is an empty summation, or
	# other structure that returns `_MA.Zero()`.
	_replace_zero($model, $expr)
	end
	return _finalize_macro(
	model,
	Containers.container_code(index_vars, indices, code, kwargs),
	__source__;
	register_name = name,
	wrap_let = true,
	)
	end

	macro expression(args...)
	_error(str...) = _macro_error(:expression, args, __source__, str...)
	args, kw_args, requested_container = Containers._extract_kw_args(args)
	if length(args) == 3
	m = esc(args[1])
	c = args[2]
	x = args[3]
	elseif length(args) == 2
	m = esc(args[1])
	c = gensym()
	x = args[2]
	else
	_error("needs at least two arguments.")
	end
	length(kw_args) == 0 \|\| _error("unrecognized keyword argument")
	if isexpr(args[2], :block)
	_error("Invalid syntax. Did you mean to use `@expressions`?")
	end
	anonvar = isexpr(c, :vect) \|\| isexpr(c, :vcat) \|\| length(args) == 2
	variable = gensym()

	idxvars, indices = Containers.build_ref_sets(_error, c)
	if args[1] in idxvars
	_error(
	"Index $(args[1]) is the same symbol as the model. Use a " *
	"different name for the index.",
	)
	end
	expr_var, build_code = _rewrite_expression(x)
	code = quote
	$build_code
	# Don't leak a `_MA.Zero` if the expression is an empty summation, or
	# other structure that returns `_MA.Zero()`.
	_replace_zero($m, $expr_var)
	end
	code =
	Containers.container_code(idxvars, indices, code, requested_container)
	# Wrap the entire code block in a let statement to make the model act as
	# a type stable local variable.
	code = _wrap_let(m, code)
	# don't do anything with the model, but check that it's valid anyway
	if anonvar
	macro_code = code
	else
	macro_code = _macro_assign_and_return(
	code,
	variable,
	Containers._get_name(c);
	model_for_registering = m,
	)
	end
	return _finalize_macro(m, macro_code, __source__)
	end

[docs] tick off macro refactoring from roadmap #3629

[docs] tick off macro refactoring from roadmap #3629

Conversation

odow commented Dec 14, 2023 • edited Loading

codecov bot commented Dec 14, 2023 • edited Loading

Codecov Report

odow commented Dec 14, 2023 • edited Loading

odow commented Dec 15, 2023 • edited Loading

mlubin commented Dec 16, 2023

odow commented Dec 16, 2023 • edited Loading

mlubin commented Dec 16, 2023

odow commented Dec 17, 2023

mlubin left a comment

Choose a reason for hiding this comment

blegat left a comment

Choose a reason for hiding this comment

odow commented Dec 19, 2023

odow commented Dec 14, 2023 •

edited

Loading

codecov bot commented Dec 14, 2023 •

edited

Loading

odow commented Dec 14, 2023 •

edited

Loading

odow commented Dec 15, 2023 •

edited

Loading

odow commented Dec 16, 2023 •

edited

Loading