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

Framework as package #1784

Merged
merged 21 commits into from
Mar 31, 2022
Merged

Conversation

joshua-cogliati-inl
Copy link
Contributor

@joshua-cogliati-inl joshua-cogliati-inl commented Mar 7, 2022


Pull Request Description

What issue does this change request address?

#1764
Closes #42

What are the significant changes in functionality due to this change request?

This changes framework to a package. This allows creating a pip package that just has a framework directory in the root directory, instead of multiple directories and files.


For Change Control Board: Change Request Review

The following review must be completed by an authorized member of the Change Control Board.

  • 1. Review all computer code.
  • 2. If any changes occur to the input syntax, there must be an accompanying change to the user manual and xsd schema. If the input syntax change deprecates existing input files, a conversion script needs to be added (see Conversion Scripts).
  • 3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
  • 4. Automated Tests should pass, including run_tests, pylint, manual building and xsd tests. If there are changes to Simulation.py or JobHandler.py the qsub tests must pass.
  • 5. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large test. If new development on the internal JobHandler parallel system is performed, a cluster test must be added setting, in XML block, the node <internalParallel> to True.
  • 6. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
  • 7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.
  • 8. If an analytic test is changed/added is the the analytic documentation updated/added?
  • 9. If any test used as a basis for documentation examples (currently found in raven/tests/framework/user_guide and raven/docs/workshop) have been changed, the associated documentation must be reviewed and assured the text matches the example.

@moosebuild
Copy link

Job Test qsubs sawtooth on c104753 : invalidated by @joshua-cogliati-inl

Failed tests/cluster_tests/InternalParallel/test_hybrid_model_code

@moosebuild
Copy link

Job Mingw Test on c104753 : invalidated by @joshua-cogliati-inl

failed in Set Python Environment

@moosebuild
Copy link

Job Test qsubs sawtooth on a3f36b8 : invalidated by @joshua-cogliati-inl

Timeout failures.

@moosebuild
Copy link

Job Test qsubs sawtooth on 366ae3b : invalidated by @joshua-cogliati-inl

Failed tests/cluster_tests/InternalParallel/test_hybrid_model_code

@joshua-cogliati-inl
Copy link
Contributor Author

joshua-cogliati-inl commented Mar 16, 2022

Suggested email:

In order to make a raven pip package, RAVEN's code needs to be inside a single package. This requires changing all the imports, since now they are inside ravenframework, instead of just at the top level. In addition, the directory framework has been renamed to ravenframework.

This will require changes in three things.

  1. Any imports of things that used to be in framework, now need to have ravenframework explicitly mentioned.
  2. Direct use of Driver.py can no longer be done, since the imports will not be setup correctly. Use raven_framework.py instead.
  3. Pickle files have the package information embedded inside, so they will need to be regenerated.

This is pull request: #1784

The following are example modifications for framework as a package in the source code.

Relative (use . if in ravenframework/, .. if in ravenframework/someDir/, ... if in ravenframework/someDir/anotherDir/ and so forth.

Example for something that is in a directory inside of ravenframework:

Original:

import Files

New:

from .. import Files

Original:

from utils import mathUtils

New:

from ..utils import mathUtils

Original:

import utils.TreeStructure

New:

from ..utils import TreeStructure

(and modify all uses of utils.TreeStructure to TreeStructure)

Original:

import utils.TreeStructure as TS

New:

from ..utils import TreeStructure as TS

Ravenframework imports:

Original:

import Files

New:

from ravenframework import Files

Original:

from utils import mathUtils

New:

from ravenframework.utils import mathUtils

Original:

import utils.TreeStructure

New:

from ravenframework.utils import TreeStructure

(and modify all uses of utils.TreeStructure to TreeStructure)

Original:

import utils.TreeStructure as TS

New:

from ravenframework.utils import TreeStructure as TS

Importlib (Example from ravenframework/Models/Code.py )

Original:

importlib.import_module("CodeInterfaces")

New:

importlib.import_module("..CodeInterfaces", "ravenframework.Models")

(Note the second parameter package anchor for finding relative packages.)

Driver.py can no longer be used from the python executable.

Original:

python framework/Driver.py "${ARGS[@]}"

New:

python raven_framework.py "${ARGS[@]}"

or

raven_framework "${ARGS[@]}"

@joshua-cogliati-inl
Copy link
Contributor Author

Before this is merged, we should send an email warning people about the upcoming change.

@PaulTalbot-INL
Copy link
Collaborator

I see there's a TEAL update in there; that's intentional? Just checking for completeness.

@joshua-cogliati-inl
Copy link
Contributor Author

I see there's a TEAL update in there; that's intentional? Just checking for completeness.

No, that should wait until after this is merged.

Copy link
Collaborator

@PaulTalbot-INL PaulTalbot-INL left a comment

Choose a reason for hiding this comment

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

I recommend we remove the TEAL update and the two commented lines; otherwise the changes pass code check.

@PaulTalbot-INL
Copy link
Collaborator

I think this PR will need @wangcj05 control board approval.

@PaulTalbot-INL
Copy link
Collaborator

I don't think these changes will impact users beyond the email Joshua prepared. However, I think this significantly impacts our approaches as developers, so I wonder if a Monday or Wednesday meeting should cover what the impact of these changes are on:

  1. Installing RAVEN (and plugins)

    1. Installation via pip
    2. Installing and running tests (separate?)
  2. Pathing within RAVEN (and plugins)

  3. ? Anything else I missed

Copy link
Collaborator

@wangcj05 wangcj05 left a comment

Choose a reason for hiding this comment

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

@joshua-cogliati-inl I have a comment about the use of "ravenframework.requestedModules" or "...." to import the modules. In addition, I think the user manual need to be updated:

  1. the places using Driver.py to rrun the input files.
  2. the code snippets that are using "import" to import raven modules

In addition, the user guide may also need some update.

@@ -23,7 +23,7 @@

#Internal Modules------------------------------------------------------------------------------------
from .MetricInterface import MetricInterface
from utils import InputData, InputTypes
from ...utils import InputData, InputTypes
Copy link
Collaborator

Choose a reason for hiding this comment

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

@joshua-cogliati-inl which is a better way to import the module, "ravenframework.utils" or "...utils"? I think I prefer to using "ravenframework.utils"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree ravenframework.utils looks nicer, but ...utils paths correctly internal to the package as a library. I'm not sure of all the reasons why, but I think importing a package as though it was outside itself it a bad practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, well, one reason I used ...utils was so I didn't have to change it when framework got renamed. Right now ravenframework.whatever is a good warning that that section of code has something strange happening to the python path (like in CodeInterfaces or in CustomModes). For this pull request I recommend keeping the relative paths, but I have no strong opinions on this for future additions.

@wangcj05
Copy link
Collaborator

I don't think these changes will impact users beyond the email Joshua prepared. However, I think this significantly impacts our approaches as developers, so I wonder if a Monday or Wednesday meeting should cover what the impact of these changes are on:

  1. Installing RAVEN (and plugins)

    1. Installation via pip
    2. Installing and running tests (separate?)
  2. Pathing within RAVEN (and plugins)

  3. ? Anything else I missed

I agree with @PaulTalbot-INL, I think next Monday is good for the discussion. @joshua-cogliati-inl Can you prepare some slides for next Monday discussion?

@joshua-cogliati-inl
Copy link
Contributor Author

I agree with @PaulTalbot-INL, I think next Monday is good for the discussion. @joshua-cogliati-inl Can you prepare some slides for next Monday discussion?

Okay, I can make some slides for next Monday's discussion.

P.S. I will look thru the manual for imports and use of Driver.

@joshua-cogliati-inl
Copy link
Contributor Author

I updated the user manual fixing places that used import and Driver.py.

@moosebuild
Copy link

Job Test qsubs sawtooth on 6d79372 : invalidated by @joshua-cogliati-inl

Timeout tests/framework/CodeInterfaceTests/RAVEN/Code Timeout tests/framework/CodeInterfaceTests/RAVEN/ROM Timeout tests/framework/CodeInterfaceTests/RAVEN/ReturnDatabase Diff tests/cluster_tests/RavenRunsRaven/Code Diff tests/cluster_tests/test_mpiqsub_parameters Diff tests/cluster_tests/AdaptiveSobol/test_parallel_adaptive_sobol

@moosebuild
Copy link

Job Test qsubs falcon on 6d79372 : invalidated by @joshua-cogliati-inl

Timeout tests/framework/CodeInterfaceTests/RAVEN/Code Timeout tests/framework/CodeInterfaceTests/RAVEN/ROM Timeout tests/framework/CodeInterfaceTests/RAVEN/ReturnDatabase Diff tests/cluster_tests/RavenRunsRaven/Code Diff tests/cluster_tests/test_mpiqsub_parameters Diff tests/cluster_tests/AdaptiveSobol/test_parallel_adaptive_sobol

Copy link
Collaborator

@wangcj05 wangcj05 left a comment

Choose a reason for hiding this comment

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

PR Looks good.

@wangcj05 wangcj05 dismissed PaulTalbot-INL’s stale review March 31, 2022 16:53

I think comments from the review have been resolved.

@wangcj05 wangcj05 mentioned this pull request Mar 31, 2022
10 tasks
@wangcj05
Copy link
Collaborator

PR is good and checklist is satisfied.

@wangcj05 wangcj05 merged commit cb6fb8f into idaholab:devel Mar 31, 2022
@wangcj05
Copy link
Collaborator

@joshua-cogliati-inl Could you send out an email to the users and plugin developers (LOGOS, TEAL, HERON, FARM, etc.)?

@wangcj05
Copy link
Collaborator

Hi @wanghy-anl, this PR causes the failure of FARM on RAVEN regression test machines. Basically, we changed the way to import raven modules. Do you have plan to update the module import in FARM? The following is the suggestion how to change the import from Josh:

Suggested email:

In order to make a raven pip package, RAVEN's code needs to be inside a single package. This requires changing all the imports, since now they are inside ravenframework, instead of just at the top level. In addition, the directory framework has been renamed to ravenframework.

This will require changes in three things.

Any imports of things that used to be in framework, now need to have ravenframework explicitly mentioned.
Direct use of Driver.py can no longer be done, since the imports will not be setup correctly. Use raven_framework.py instead.
Pickle files have the package information embedded inside, so they will need to be regenerated.
This is pull request: https://github.com/idaholab/raven/pull/1784

The following are example modifications for framework as a package in the source code.

Relative (use . if in ravenframework/, .. if in ravenframework/someDir/, ... if in ravenframework/someDir/anotherDir/ and so forth.

Example for something that is in a directory inside of ravenframework:

Original:

import Files

New:

from .. import Files

Original:

from utils import mathUtils

New:

from ..utils import mathUtils

Original:

import utils.TreeStructure

New:

from ..utils import TreeStructure

(and modify all uses of utils.TreeStructure to TreeStructure)

Original:

import utils.TreeStructure as TS

New:

from ..utils import TreeStructure as TS

Ravenframework imports:

Original:

import Files

New:

from ravenframework import Files

Original:

from utils import mathUtils

New:

from ravenframework.utils import mathUtils

Original:

import utils.TreeStructure

New:

from ravenframework.utils import TreeStructure

(and modify all uses of utils.TreeStructure to TreeStructure)

Original:

import utils.TreeStructure as TS

New:

from ravenframework.utils import TreeStructure as TS

Importlib (Example from ravenframework/Models/Code.py )

Original:

importlib.import_module("CodeInterfaces")

New:

importlib.import_module("..CodeInterfaces", "ravenframework.Models")

(Note the second parameter package anchor for finding relative packages.)

Driver.py can no longer be used from the python executable.

Original:

python framework/Driver.py "${ARGS[@]}"

New:

python raven_framework.py "${ARGS[@]}"

or

raven_framework "${ARGS[@]}"

@wanghy-anl
Copy link
Contributor

wanghy-anl commented May 12, 2022 via email

@wangcj05
Copy link
Collaborator

wangcj05 commented May 12, 2022 via email

@wanghy-anl wanghy-anl mentioned this pull request May 13, 2022
9 tasks
@wanghy-anl
Copy link
Contributor

wanghy-anl commented May 13, 2022 via email

@wangcj05
Copy link
Collaborator

wangcj05 commented May 13, 2022 via email

@wangcj05 wangcj05 mentioned this pull request May 19, 2022
9 tasks
@wangcj05 wangcj05 added the RAVENv2.2 for RAVENv2.2 Release label Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RAVENv2.2 for RAVENv2.2 Release Ready To Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistent Module Imports
5 participants