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

Updating to newer python #1933

Merged
merged 17 commits into from
Sep 1, 2022

Conversation

joshua-cogliati-inl
Copy link
Contributor

@joshua-cogliati-inl joshua-cogliati-inl commented Aug 8, 2022


Pull Request Description

What issue does this change request address?

#1748
see #1806 and #1764

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

Updating dependencies to update python version. It also regolds some tensorflow files and adds a new min_python_version to test specifications.


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.

@joshua-cogliati-inl
Copy link
Contributor Author

Hm, the windows tests are failing because cbcpy from LOGOS is not available for python > 3.7.

@joshua-cogliati-inl
Copy link
Contributor Author

The typical other failures are:

FAILED:
Diff tests/framework/Samplers/AdaptiveBatch/ThickenedBand
Failed tests/framework/unit_tests/utils/Debugging
Diff tests/framework/ROM/tensorflow_keras/tf_cnn1d
Diff tests/framework/ROM/tensorflow_keras/tf_lstm
Diff tests/framework/ROM/tensorflow_keras/tf_mlpr
Diff tests/framework/ROM/tensorflow_keras/LSTMFromFile

Basically, these need to be regolded.

@dgarrett622
Copy link
Collaborator

Hm, the windows tests are failing because cbcpy from LOGOS is not available for python > 3.7.

I couldn't find anywhere that cpcpy is actually used. I think it was intended to be a workaround to installing the CBC solver on Windows to work with pyomo since conda doesn't support a Windows installation. I think the dependency should be removed (easier) and abandon the use of CBC on Windows (unless the user goes through the process of installing CBC from the binaries like me, not recommended), or additional work done to integrate cbcpy with pyomo (more work).

@moosebuild
Copy link

Job Precheck on 1147bea : invalidated by @joshua-cogliati-inl

Could not resolve host: github.com

1 similar comment
@moosebuild
Copy link

Job Precheck on 1147bea : invalidated by @joshua-cogliati-inl

Could not resolve host: github.com

@moosebuild
Copy link

Job Test CentOS 7 on 26af206 : invalidated by @joshua-cogliati-inl

failed in set python environment

@moosebuild
Copy link

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

failed in fetch

@joshua-cogliati-inl
Copy link
Contributor Author

joshua-cogliati-inl commented Aug 8, 2022

Hm, the windows tests are failing because cbcpy from LOGOS is not available for python > 3.7.

I couldn't find anywhere that cpcpy is actually used. I think it was intended to be a workaround to installing the CBC solver on Windows to work with pyomo since conda doesn't support a Windows installation. I think the dependency should be removed (easier) and abandon the use of CBC on Windows (unless the user goes through the process of installing CBC from the binaries like me, not recommended), or additional work done to integrate cbcpy with pyomo (more work).

Considering that the result of running the LOGOS windows tests with cbcpy with python 3.7 is:

08-Aug-22 09:57:38 pyomo.opt            ERROR    Solver (cbc) returned non-zero return code (3221225781)

Traceback (most recent call last):

  File "C:\Git\opt\build_root\raven\plugins\LOGOS\src\logos_main.py", line 72, in <module>

    modelInstance.run()

  File "C:\Git\opt\build_root\raven\plugins\LOGOS\src\CapitalInvestments\PyomoModels\ModelBase.py", line 294, in run

    results = opt.solve(model, load_solutions=False, tee=self.tee, **{'use_signal_handling':False})

  File "C:\Users\cogljj\.conda\envs\raven_libraries\lib\site-packages\pyomo\opt\base\solvers.py", line 597, in solve

    "Solver (%s) did not exit normally" % self.name)

pyomo.common.errors.ApplicationError: Solver (cbc) did not exit normally

dropping it is definitely worth trying.

@moosebuild
Copy link

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

failed in fetch

type = OrderedCSV
output = 'data/outCon1DClassifier.csv'
#note, mac uses tensorflow 2.7 but others use 2.3
mac_gold = 'mac/data/outCon1DClassifier.csv'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgarrett622 Thanks for adding the os specific gold files :)

@joshua-cogliati-inl
Copy link
Contributor Author

This pull request depends on idaholab/LOGOS#42

@moosebuild
Copy link

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

computer rebooted

@joshua-cogliati-inl
Copy link
Contributor Author

This pull request depends on idaholab/LOGOS#42

Which now has been merged, so this is ready to review.

@moosebuild
Copy link

Job Test Ubuntu 18 PIP on 1e0d4d1 : invalidated by @joshua-cogliati-inl

restarted civet

@moosebuild
Copy link

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

FAILED: 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.

@joshua-cogliati-inl I have reviewed the changes, and I have following comments

  1. It seems our tests for tensor flow is not reliable, the values for test keras_mlp_regression change significantly
  2. Do you know why the tests for Adaptive Batch need to be regold?

@joshua-cogliati-inl
Copy link
Contributor Author

joshua-cogliati-inl commented Aug 29, 2022

The tensorflow data still follows the general trend:
tensor_flow_comparison
I agree that it would be nice if it was stabler.

I am not sure why the Adaptive batch is different, but the sampler returned different values, which resulted in the limit surface being somewhat different. (If the limit surface were perfect, both would be circles)

limit_surface_comparison

@wangcj05
Copy link
Collaborator

The result for tensorflow looks good to me. For the other test, which one is the original results for the limit surface test, the orange color? I'm curious why the samples get changed, Do you see any libraries versions difference?

@joshua-cogliati-inl
Copy link
Contributor Author

The result for tensorflow looks good to me. For the other test, which one is the original results for the limit surface test, the orange color? I'm curious why the samples get changed, Do you see any libraries versions difference?

Yes, the orange were the original. I am not sure why the samples got changed. The adaptive batch doesn't seem to be using tensorflow, and other libraries (including scipy 1.5.3 and scikit-learn 0.24.2) are generally the same.

@wangcj05
Copy link
Collaborator

Both results look reasonable for me. But I think if you have some extra time I would recommend to take a deep look at the code and figure out the cause for the difference.

@joshua-cogliati-inl
Copy link
Contributor Author

Both results look reasonable for me. But I think if you have some extra time I would recommend to take a deep look at the code and figure out the cause for the difference.

I haven't figured out the exact cause yet, but it is in Samplers/LimitSurfaceSearch.py

@joshua-cogliati-inl
Copy link
Contributor Author

joshua-cogliati-inl commented Sep 1, 2022

More details. In Samplers/LimitSurfaceSearch.py it is finding different values for self.surfPoint[maxGridId][maxId,:]

    if self.surfPoint is not None and len(self.surfPoint) > 0:
      if self.batchStrategy == 'none':
        self.__scoreCandidates()
        maxDistance, maxGridId, maxId =  0.0, "", 0
        for key, value in sorted(self.invPointPersistence.items()):
          if key != self.exceptionGrid and self.surfPoint[key] is not None:
            localMax = np.max(self.scores[key])
            if localMax > maxDistance:
              maxDistance, maxGridId, maxId  = localMax, key,  np.argmax(self.scores[key])
        if maxDistance > 0.0:
          for varIndex, _ in enumerate([key.replace('<distribution>','') for key in self.axisName]):
            self.values[self.axisName[varIndex]] = copy.copy(float(self.surfPoint[maxGridId][maxId,varIndex]))
            self.inputInfo['SampledVarsPb'][self.axisName[varIndex]] = self.distDict[self.axisName[varIndex]].pdf(self.values[self.axisName[varIndex]])
            self.inputInfo['ProbabilityWeight-'+self.axisName[varIndex]] = self.distDict[self.axisName[varIndex]].pdf(self.values[self.axisName[varIndex]])
          varSet=True
        else:
          self.raiseADebug('Maximum score is 0.0')

For the place where there are differences, self.scores has multiple values that are the max, so which one found is changing.

@wangcj05
Copy link
Collaborator

wangcj05 commented Sep 1, 2022

checklist is good, tests are green.

@wangcj05 wangcj05 merged commit 8d064cb into idaholab:devel Sep 1, 2022
@joshua-cogliati-inl joshua-cogliati-inl deleted the update_to_newer_python branch September 2, 2022 13:47
j-bryan pushed a commit to j-bryan/raven that referenced this pull request Sep 26, 2022
* Trying with tensorflow 2 float.

* list size varies between python versions.

* Testing removing cbcpy.

* Regolding tests/framework/Samplers/AdaptiveBatch/ThickenedBand

* Regolding tensorflow tests.

* Trying specifying tensorflow version.

* Use 2.4 on mac and 2.3 on linux and windows.

* Adding min_python_version check in rook and using for test

* Fix pylint problems.

* Mac needs a different version of tensorflow.

* Switching to different gold file.

* If gold file not found, print out expected filename.

* Revert LOGOS change.

* Updating LOGOS and HERON submodules.

* Forcing version of protobuf because of ray incompatibility.

Co-authored-by: civet runner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants