Skip to content

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.

//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?

// Test that we differentiate both 'x' and 'y' correctly when x is a
// predecessor of y.

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