Bugfix when copying the ExperiemntData object and add warning to _retrieve_data

[ItamarGoldman]

Summary

As said in #1314 , copy method in Experiment Data class will now copy the provider attribute to the copied instance and warnning if data isn't retrieved due to provider=None .

Details and comments

Some details that should be in this section include:

• Why this change was necessary
  This is a bug that the provider isn't copied. In addition, _retrive_data() method in the ExperimentData class doesn;t retirive data if provider is not provided. Added a warning so the user will know.

[yaelbh]

It all started with #1314. I understand that you did some debugging, which revealed that the cause of #1314 was that providers were not copied.

Is there a way to add a test that reproduces the code snippet of #1314, and verifies that the issue has really been resolved?

[yaelbh]

1. Why are you copying _result_data?
2. Why did you add the and not self._result_data.copy() section? It makes the code continue where previously it returned.
3. The warning is not clear. Replace it with something similar to what you wrote in the release notes, which is rephrased much better.

[ItamarGoldman]

1. Because _result_data can be locked so I used the copy method.
2. I used and not self._result_data.copy() so it will not print the warning unnecessarily. If there is no provider but there are results in self._result_data, the warning is meaningless.
3. What about Provider for ExperimentData object doesn't exist. Failed to retrieve data from the server. ?

[yaelbh]

Can you please elaborate a bit about what _result_data is, and when it is expected to be populated, is there a scenario (now that the provider is copied, and assuming that there's no other bug) that the provider is None? You can answer some of these questions in inline comments. In general if you can please explain the general picture here.

1. What do you mean by "locked", and how is copying related?
2. After the if come not only the warning but also the return statement, which was there also before this PR. So it looks like, in addition to the warning, there is also a change in behavior introduced here.
3. Looks good. You're not mentioning the _result_data thing, but I don't fully understand what it is so can't tell if it's necessary to include it in the warning message.

[ItamarGoldman]

1. _result_data is an attribute of ExperimentData of type ThreadSafeList() and is used as a list of Result object. As a thread safe object, whenever _result_data is being is being accessed (append data, read data, etc) it is needed to be locked (Source for working with safe thread obj here). The copy method is taking care of it automatically so I used it for clear code reading.
2. True, I changed it to

  if self.provider is None:
      if not self._result_data.copy():
          # Adding warning so the user will have indication why the analysis may fail.
          LOG.warning("Warning Text.")
      return

3. I am not sure if to include it in the warning as this warning is there to indicate the user that the problem in analysis could be due to missing data. What about the following sentence? "Provider for ExperimentData object doesn't exist, resulting in a failed attempt to retrieve data from the server ; no stored result data exists"

[yaelbh]

(3) is very good. Only remove the space between "server" and the semicolon that follows it.

(2) is also good.

I still don't understand (1). Is _result_data already locked, and for some reason you want to overcome it, and the copying helps you with this? Or does copy lock, in this case, why do you want to lock? And when will it be unlocked? And what's locked - the original or the copy? Anyhow this should be explained in a code comment.

[yaelbh]

I understand. The original _result_data is locked, therefore the check if _result_data is impossible. So you copy it, the new object is not locked and you can check if _result_data.copy(). So just please an inline comment in the code.

[yaelbh]

These items have to be completed for approval:

1. Note my previous comment about connection to the original issue.
2. An answer from @coruscating about the release notes.
3. And of course what remains from the other comments.

[yaelbh]

Can you please elaborate a bit about what _result_data is, and when it is expected to be populated, is there a scenario (now that the provider is copied, and assuming that there's no other bug) that the provider is None? You can answer some of these questions in inline comments. In general if you can please explain the general picture here.

1. What do you mean by "locked", and how is copying related?
2. After the if come not only the warning but also the return statement, which was there also before this PR. So it looks like, in addition to the warning, there is also a change in behavior introduced here.
3. Looks good. You're not mentioning the _result_data thing, but I don't fully understand what it is so can't tell if it's necessary to include it in the warning message.

[yaelbh]

It all started with #1314. I understand that you did some debugging, which revealed that the cause of #1314 was that providers were not copied.

Is there a way to add a test that reproduces the code snippet of #1314, and verifies that the issue has really been resolved?

[ItamarGoldman]

I added to a test under test_copy_metadata method in test_db_experiment_data.py. There I checked if the provider of the experiment data is copied. In FakeBackend class I put provider=FakeProvider() in the initialization.

[yaelbh]

Please run locally in your environment the code snippet of #1314. If it works then you can merge.