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

Update Examples to track IDAES deprecation warnings #9

Open
jsiirola opened this issue Feb 24, 2022 · 13 comments
Open

Update Examples to track IDAES deprecation warnings #9

jsiirola opened this issue Feb 24, 2022 · 13 comments
Assignees
Labels
Priority:Normal Normal Priority Issue or PR

Comments

@jsiirola
Copy link

While putting together IDAES/examples-pse#102, I ran across a ton of deprecation warnings in the example, e.g.:

2022-02-24 12:30:05 [WARNING] idaes.power_generation.unit_models.boiler_heat_exchanger: 'DeltaTMethod' is deprecated use 'HeatExchangerFlowPattern' This will be removed in IDAES 3.0
2022-02-24 12:30:05 [WARNING] idaes.power_generation.unit_models.boiler_heat_exchanger: Config item delta_T_method is deprecated use flow_pattern. Will be removed in IDAES 3.0.
2022-02-24 12:30:05 [WARNING] idaes.power_generation.unit_models.boiler_heat_exchanger: 'DeltaTMethod' is deprecated use 'HeatExchangerFlowPattern' This will be removed in IDAES 3.0
2022-02-24 12:30:05 [WARNING] idaes.power_generation.unit_models.boiler_heat_exchanger: Config item side_1_property_package is deprecated. Will be removed in IDAES 3.0.
2022-02-24 12:30:05 [WARNING] idaes.power_generation.unit_models.boiler_heat_exchanger: Config item side_2_property_package is deprecated. Will be removed in IDAES 3.0.

and

WARNING: DEPRECATED: DEPRECATED: svg_tag, the tags, tag_format and
    tag_format_default arguments are deprecated use tag_group instead.
    (deprecated in 1.12) (called from <ipython-input-8-b95b72557475>:6)
WARNING: DEPRECATED: DEPRECATED: svg_tag, the tags, tag_format and
    tag_format_default arguments are deprecated use tag_group instead.
    (deprecated in 1.12) (called from <ipython-input-8-b95b72557475>:7)

We should revisit the examples and update them to track the current recommended API.

We should also consider updating the examples tests to fail on (unexpected) warnings.

@eslickj
Copy link
Member

eslickj commented Feb 24, 2022

Which model is this? Is it imported from idaes-pse or all in the example?

@jsiirola
Copy link
Author

The example is src/Examples/Advanced/DataRecon/econ_parmest_testing.ipynb, importing

from idaes.power_generation.unit_models.boiler_heat_exchanger import (
    BoilerHeatExchanger, 
    TubeArrangement, 
    DeltaTMethod
)

@eslickj
Copy link
Member

eslickj commented Feb 24, 2022

Ok, so it's the notebook that needs to be updated. I assume you are working on this now, so I need to wait to update it.

@bpaul4
Copy link
Contributor

bpaul4 commented Feb 28, 2022

When debugging notebooks and writing new examples, I encounter many deprecation warnings concerning missing scaling factors or not specifying an equation form for Perry's liquid density methods. The second one is from one of my PR's to add a second equation form and default to the first/original equation form is no flag is provided. @andrewlee94 would it be best to update our examples so these warnings do not appear, and if so would you support catching these warnings via tests as @jsiirola suggests above?

@andrewlee94
Copy link
Member

@bpaul4 Yes, we should be fixing these deprecation warnings and it would be good to have a check for them. This will be especially important over the next few months as we begin the transition to IDAES v2.0.

@eslickj
Copy link
Member

eslickj commented Mar 1, 2022

The scaling factors aren't deprecation warnings. Just warnings. By popular request, I'm planning to add an option to silence the scaling warnings.

@andrewlee94
Copy link
Member

As John noted those the scaling factor messages are warning, but the correct path here is to fix these rather than silence them - we need to show users how to use the scaling tools and this is the prefect opportunity.

@ksbeattie ksbeattie added the Priority:High High Priority Issue or PR label Mar 10, 2022
@eslickj
Copy link
Member

eslickj commented Aug 6, 2022

PR IDAES/idaes-pse#822 can turn deprecation warnings into exceptions, which can help track down the exact location of deprecation warnings being generated, and help us make sure we get them all.

@andrewlee94
Copy link
Member

I think we should have fixed all the deprecations that were being raised, so now it should just be a matter of adding a test to warn us when new deprecations appear.

@lbianchi-lbl
Copy link
Contributor

lbianchi-lbl commented Dec 8, 2022

  • Implementing the check at runtime (e.g. the way that deprector does it) is trickier for Jupyter notebooks, since they run in their own isolated process and the runtime is not accessible for patching
  • One alternative would be to execute the notebooks and parse the output cells of the notebook post-execution to verify that it does not contain deprecation messages
  • Either way, it's not clear if it makes sense to try to implement the check in this repository since we're about to migrate to the new IDAES/examples repo

@ksbeattie ksbeattie transferred this issue from IDAES/examples-pse Dec 15, 2022
@ksbeattie ksbeattie moved this to In Progress in 2023 May Release Feb 16, 2023
@dangunter
Copy link
Member

fwiw, in this (examples) repository, we could implement this check by using the same function used in the pre-processing step to find all notebooks (ie idaes_examples.util.find_notebooks()) and parse them. We would just need to make sure it ran after all the other tests -- there is a plugin called pytest-order we can use.

@lbianchi-lbl
Copy link
Contributor

Summarizing the discussion from the dev call:

  • @lbianchi-lbl will take a stab at creating a pytest plugin that looks at the .ipynb post-execution output cells and does a text search for DEPRECATED (must ensure that this check is run after the notebooks are run).

@lbianchi-lbl lbianchi-lbl moved this from In Progress to Todo in 2023 May Release May 4, 2023
@lbianchi-lbl
Copy link
Contributor

This is still relevant. I might be able to make some progress on this before the Feb release.

@ksbeattie ksbeattie added Priority:Normal Normal Priority Issue or PR and removed Priority:High High Priority Issue or PR labels Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

No branches or pull requests

7 participants