Skip to content

Conversation

@manuyavuz
Copy link

@manuyavuz manuyavuz commented Jul 5, 2021

Previous code was locking the requests from tb's is_active method until _is_active is set.
However, it was also missing setting it to false when there is no related trace files.

This fix basically sets the value to false if generator returns empty list, which will unlock tb request and return false.

Fixes #334, tensorflow/tensorboard#5076, tensorflow/tensorboard#5111.

Previous code was locking the requests from tb's `is_active` method until `_is_active` is set.
However, it was also missing setting it to false when there is no related trace files.

This fix basically sets the value to false if generator returns empty list, which will unlock tb request and return false.
@guotuofeng
Copy link
Contributor

This is similar with #326. The fix is conflict with #130

@guotuofeng
Copy link
Contributor

@manuyavuz
Copy link
Author

@guotuofeng thanks for the feedback.

Indeed it was the same fix with #326, sorry for the dupe.

I understand that team wants to handle dynamic changes in the logdir after the plugin starts more properly (deletions do not seem to be handled right now for instance). I think that threading Event lock can even be completely removed with a better architecture. However, it might make this issue linger for a much longer period of time. Do you have an ETA for when would team implement the new exhaustive solution that would also fix this issue?

It seems to me that this fix or #326 can act as a minimal hotfix change for a quick 0.2.1 release according to the project's release process. What do you think?

@guotuofeng
Copy link
Contributor

If the is_active is set to false and there is some available data later, we have to manually press F5 in browser to allow the plugin be shown.

On another hand, we are considering to make the is_active always return true to fix this issue and we don't have time to fully test it.
Since the severity/priority of this issue is not so much high, we plan to fix this issue in next release after 0.2.1.

@manuyavuz-pointr
Copy link

I recognized the need for force refresh in my testing too.

Also it makes much more sense to have an always active plugin and handle no data gracefully.

I can live with this for now, thanks for the information!

@guotuofeng guotuofeng closed this Jul 7, 2021
@guotuofeng
Copy link
Contributor

guotuofeng commented Jul 8, 2021

@manuyavuz, the #341 should be able to fix the bug.

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.

No results loaded with torch-tb-profiler when there isn't a profiler in the code

4 participants