Skip to content

cached_session for graph tests #1172

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

Merged
merged 3 commits into from
Sep 14, 2023

Conversation

novikov-alexander
Copy link
Contributor

@novikov-alexander novikov-alexander commented Sep 13, 2023

Implementation of cached_session for the possibility of further porting of tests.

cached_session is a singleton which is constructed on the first call and disposed on the tests cleanup.

There's also testBoundaryContinue() test implemented to prove that cache_session() works.

@@ -29,7 +30,7 @@ private void _testWhileContextHelper(int maximum_iterations)
var b = new Func<Tensor, Tensor>(x => math_ops.add(x, 1, name: "c"));
//control_flow_ops.while_loop(
// c, b, i , maximum_iterations: tf.constant(maximum_iterations));
foreach (Operation op in sess.graph.get_operations())
foreach (Operation op in sess.Single().graph.get_operations())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Oceania2018 I'm not sure how exactly IEnumerable should work there since Python generator produces only one instance. Anyway this test wasn't working and it is still not implemented yet.

Copy link
Member

Choose a reason for hiding this comment

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

The test didn't come through.

   Test method TensorFlowNET.UnitTest.Gradient.GradientTest.testBoundaryContinue threw exception: 
Tensorflow.ValueError: Cannot use the default session to evaluate tensor: the tensor's graph is different from the session's graph. Pass an explicit session to `eval(session=sess)`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, it was flaky. Now it should work.

Copy link
Collaborator

@Wanglongzhi2001 Wanglongzhi2001 Sep 14, 2023

Choose a reason for hiding this comment

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

@Oceania2018 I'm not sure how exactly IEnumerable should work there since Python generator produces only one instance. Anyway this test wasn't working and it is still not implemented yet.

Since the different behavior of yield in Python and C#, If cached_session only need to have one session, I think it is better not to use yield? And then you can use using to replace with in GradientTest.cs

Copy link
Contributor Author

@novikov-alexander novikov-alexander Sep 14, 2023

Choose a reason for hiding this comment

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

Could, please, explain me why yield used in Python in this case?
Anyway fixed.

{
if (self._cached_session != null)
{
self._cached_session.Dispose();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Oceania2018 should we cleanup the graph and the config as well?


self.cached_session();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Oceania2018 I'm not sure how exactly "with" should be ported there, since "IEnumerable" isn't "IDisposable" and we don't need to call any "Dispose" for the enumerable itself. I guess session's "Dispose" should be called automatically.

@Oceania2018 Oceania2018 added the enhancement New feature or request label Sep 14, 2023
@Oceania2018 Oceania2018 merged commit 9ddff69 into SciSharp:master Sep 14, 2023
@novikov-alexander novikov-alexander deleted the alnovi/cached_session branch November 9, 2023 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants