Skip to content

First outline of fitting result models#241

Closed
jonc125 wants to merge 13 commits intomasterfrom
203-fitting-result
Closed

First outline of fitting result models#241
jonc125 wants to merge 13 commits intomasterfrom
203-fitting-result

Conversation

@jonc125
Copy link
Copy Markdown
Contributor

@jonc125 jonc125 commented Jan 21, 2020

Done as a thinking aloud kind of exercise. I think this is along the right lines, but I'm not sure we want to start from here!

The reason is that we may wish to refactor the Experiment model first so it refers to CachedProtocolVersion and CachedModelVersion instead of storing string SHA values in protocol_version and model_version. When it was originally written, the repocache didn't exist. And then when we first added it we assumed it could be fully ephemeral and just regenerated as needed. But increasingly it is storing data that is expensive to recompute, and the populate method ensures that cache entries are only removed if the corresponding commit is removed, so it is as permanent as our knowledge of the underlying repo. If the commit is removed, we probably do want to remove linked experiment results.

What do you think @sroderick-g5sro ?

Then related thoughts building on this...

  • Assuming we do that, we'd want to do the same with FittingResult here.
  • I like the Runnable abstraction. In particular, it means that the RunningExperiment table can remain as-is, and indeed receipt of finished experiments in experiments/processing.py:process_callback (and cancelling running experiments) doesn't need any additions for fitting experiments!
  • To get submission of fitting experiments to work we'll need a 'fitting' version of experiments/processing.py:submit_experiment that creates the FittingResult etc instances. We may be able to refactor so they share some code though.
  • I don't think we want to try automatically migrating existing hacky fitting specs (stored as Protocol instances) to FittingSpec instances etc. Instead manually migrate ones we care about. It seems likely to be too difficult to split the data & spec & protocol file out into sensible places. Would you agree @mirams ?
  • We'll need to consider how best to test this as we develop, including building enough of the fitting experiment submission UI to test it out manually.

@mirams
Copy link
Copy Markdown
Contributor

mirams commented Jan 21, 2020

Yeah, no need to migrate the old hacky specs, there is pretty much only one that people copied and pasted and nobody's using it in anger!

@helenst
Copy link
Copy Markdown
Contributor

helenst commented Aug 21, 2020

I'm using this as a starting point for work on #134 although since become outdated, a rebase is required. @jonc125 are you happy for me to overwrite / rebase this branch? :)

@jonc125
Copy link
Copy Markdown
Contributor Author

jonc125 commented Aug 21, 2020

You might find it easier to create a new branch off develop rather than rebasing? But I'm happy either way!

Some models in repocache are now referenced using names to avoid
circular imports
@helenst helenst force-pushed the 203-fitting-result branch 2 times, most recently from a28a57b to d3f66aa Compare August 25, 2020 12:08
@helenst helenst force-pushed the 203-fitting-result branch from d3f66aa to 795784d Compare August 25, 2020 13:07
@helenst helenst force-pushed the 203-fitting-result branch from bbf1eab to 900157e Compare August 26, 2020 15:23
@helenst helenst force-pushed the 203-fitting-result branch from 59fc29c to c83d30b Compare August 27, 2020 11:26
@helenst helenst force-pushed the 203-fitting-result branch from 5f063a3 to 27df52b Compare August 27, 2020 12:15
Fitting result URLs get their own subnamespace within the fitting app
@helenst helenst force-pushed the 203-fitting-result branch 5 times, most recently from f894208 to f926146 Compare August 31, 2020 10:20
@helenst helenst force-pushed the 203-fitting-result branch from f926146 to 5df21fe Compare August 31, 2020 11:51
@helenst
Copy link
Copy Markdown
Contributor

helenst commented Sep 1, 2020

Closing so I can create new PR on this branch under my username...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants