Skip to content

Add view to run experiments based on a given entity version#138

Merged
skeating merged 59 commits intomasterfrom
67-add-view-run-exp
Jun 12, 2019
Merged

Add view to run experiments based on a given entity version#138
skeating merged 59 commits intomasterfrom
67-add-view-run-exp

Conversation

@skeating
Copy link
Copy Markdown
Contributor

@skeating skeating commented May 9, 2019

Fixes #67.

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #138 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #138   +/-   ##
=====================================
  Coverage      95%    95%           
=====================================
  Files          50     50           
  Lines        2069   2069           
  Branches      224    224           
=====================================
  Hits         1966   1966           
  Misses         76     76           
  Partials       27     27

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94a66d6...f9e07bf. Read the comment docs.

@codecov-io
Copy link
Copy Markdown

codecov-io commented May 9, 2019

Codecov Report

Merging #138 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #138     +/-   ##
=======================================
+ Coverage    91.8%    92%   +0.1%     
=======================================
  Files          60     60             
  Lines        2418   2465     +47     
  Branches      253    262      +9     
=======================================
+ Hits         2222   2269     +47     
  Misses        169    169             
  Partials       27     27
Impacted Files Coverage Δ
weblab/entities/urls.py 100% <ø> (ø) ⬆️
weblab/entities/templatetags/entities.py 100% <100%> (ø) ⬆️
weblab/entities/views.py 94.1% <100%> (+0.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e883b33...a4dbf1c. Read the comment docs.

Copy link
Copy Markdown
Contributor

@jonc125 jonc125 left a comment

Choose a reason for hiding this comment

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

Hi Sarah. Some initial thoughts from a quick read-through; hope they're helpful!

Comment thread weblab/entities/tests/test_views.py
Comment thread weblab/entities/views.py Outdated
Comment thread weblab/entities/views.py Outdated
Comment thread weblab/entities/views.py Outdated
Comment thread weblab/entities/views.py
Comment thread weblab/templates/entities/entity_runexperiments.html Outdated
Comment thread weblab/templates/entities/entity_runexperiments.html Outdated
Comment thread weblab/templates/entities/entity_runexperiments.html Outdated
Comment thread weblab/templates/entities/entity_version.html Outdated
Comment thread weblab/templates/entities/entity_version.html Outdated
@skeating skeating requested a review from jonc125 May 31, 2019 08:59
@jonc125
Copy link
Copy Markdown
Contributor

jonc125 commented Jun 1, 2019

This could do with a test that the view now works with a non-latest version of 'this_entity', but apart from that I think it's ready to merge.

@jonc125
Copy link
Copy Markdown
Contributor

jonc125 commented Jun 3, 2019

Note there are some fixes for this now as part of #185, so we'll need to cherry-pick those into this branch before merging. Relevant commits are I think 74c5b77, 7e4d598, 3208fc6.

Also, given the speed of this view at the workshop was a little slow and the display was rather long, we might want to make a couple of extra changes.

  • More compact display. E.g. here, I think we want ideally 2 lines per version, something like:
    • <a><b>tag</b> <small>(sha...)</small></a> <small>by Author created date-time</small>
    • <small>commit message</small>
      It may need some style="display: inline;" attributes added on the small elements?
  • See if we can reduce the number of DB queries needed (and especially make sure no calls to git are made) - if you have LOG_DEBUG=True you should see queries output. Play with select_related and prefetch_related.

@skeating
Copy link
Copy Markdown
Contributor Author

skeating commented Jun 7, 2019

Okay I cannot change the code that loads the runexperiments view to not grab commits from the repo - and this will be heavy hitting as it queries each version of each entity !
See #191 for more detailed explanation.

I also cannot work out a way of fetching the commits without hitting the repo. I can get cachedentities with some of the information but I need the actual commit object for the hrefs call to url functions.

I accept I may be missing something but have spent some time playing with various queries etc and cannot see it.

@skeating
Copy link
Copy Markdown
Contributor Author

I would claim this done - again :-)

Copy link
Copy Markdown
Contributor

@jonc125 jonc125 left a comment

Choose a reason for hiding this comment

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

It just needs a test adding (or more likely modify one of the existing tests) to demonstrate that the view now works with a non-latest version of 'this_entity'.

skeating added 2 commits June 11, 2019 11:44
which was worth it as I discovered I always redirected back to latest version even when using another - this is now fixed and tested
@skeating
Copy link
Copy Markdown
Contributor Author

Might really be done this time :-)

@jonc125 jonc125 self-requested a review June 11, 2019 14:42
@jonc125
Copy link
Copy Markdown
Contributor

jonc125 commented Jun 11, 2019

Yep :)

@skeating skeating merged commit 39bf5d8 into master Jun 12, 2019
@skeating skeating deleted the 67-add-view-run-exp branch June 12, 2019 13:55
MauriceHendrix pushed a commit that referenced this pull request Jul 22, 2021
Add view to run experiments based on a given entity version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add view to run experiments based on a given entity version

4 participants