Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/memote #162

Merged
merged 36 commits into from
Jul 1, 2021
Merged

Feat/memote #162

merged 36 commits into from
Jul 1, 2021

Conversation

BenjaSanchez
Copy link
Contributor

Main improvements in this PR:

The memote report has been added as a CI tool for assessing changes in the model. For now it looks messy, but most of those problems have to do with the history report generation, so it would not be a problem to merge now. The one thing that worries me is that each .json file is ~2 MB, which as we continue development might escalate a bit too fast. @Midnighter @ChristianLieven any ideas on how to reduce the size of those files?

I hereby confirm that I have:

  • Tested my code with all requirements for running the model
  • Selected devel as a target branch (top left drop-down menu)
  • If needed, asked first in the Gitter chat room about this PR

@BenjaSanchez BenjaSanchez added enhancement new field/feature memote anything related to the memote report wip work in progress labels Sep 20, 2018
@BenjaSanchez BenjaSanchez self-assigned this Sep 20, 2018
@BenjaSanchez BenjaSanchez removed the wip work in progress label Sep 26, 2018
@BenjaSanchez BenjaSanchez added the wip work in progress label Sep 26, 2018
@BenjaSanchez
Copy link
Contributor Author

@Midnighter after switching from JSON to SQLite (as suggested), the size of the report decreased roughly by half (from 2.04 MB down to 1.19 MB); however, the size still increases linearly with the number of commits: a second test commit increased the size of results.db from 1.19 MB to 2.37 MB. This would still make the size of the repo grow quite fast if we merge to devel. Is there any other solution to this? If needed I can open an issue at memote for further discussion.

In the meantime, this PR will remain open until (at least) opencobra/memote#500 is addressed, as it would help out significantly with the amount of reports generated. In turn, opencobra/memote#498 should be addressed before running memote history on the whole repo (but that can be done on another branch).

@BenjaSanchez BenjaSanchez added standby work somewhere else is needed before advancing and removed wip work in progress labels Nov 12, 2018
@Midnighter
Copy link

While opencobra/memote#498 would be a big improvement, I don't think it's strictly necessary. Or what do you think?

@mihai-sysbio
Copy link
Member

@Midnighter, personally I think the more decoupled memote is from the structure of a repository, the easier it is to use in different automated setups. So I would go even further than opencobra/memote#498, by allowing the user to decide what the report should include instead of forcefully running the entire history. However, as you say, this is not strictly necessary here.

Since #239 builds on the work here and is ready for merging, I think the current PR can safely be closed.

@Midnighter
Copy link

@Midnighter, personally I think the more decoupled memote is from the structure of a repository, the easier it is to use in different automated setups. So I would go even further than opencobra/memote#498, by allowing the user to decide what the report should include instead of forcefully running the entire history. However, as you say, this is not strictly necessary here.

Since #239 builds on the work here and is ready for merging, I think the current PR can safely be closed.

Could you open an issue on memote, please, outlining what you would like to be able to specify and how you envision that to work?

@mihai-sysbio
Copy link
Member

Could you open an issue on memote, please, outlining what you would like to be able to specify and how you envision that to work?

I tried to give a comprehensive description in opencobra/memote#715.

@mihai-sysbio
Copy link
Member

From the last Action log I see everything worked up until pushing to gh-pages. That branch seems to be protected - how can we make that work with the memote reports?

@mihai-sysbio mihai-sysbio removed the standby work somewhere else is needed before advancing label Apr 28, 2021
@mihai-sysbio mihai-sysbio marked this pull request as ready for review April 28, 2021 11:09
@edkerk
Copy link
Member

edkerk commented Jun 29, 2021

@mihai-sysbio Should the gh-pages issue not be resolved before merging? Although I cannot find a solution for it...

@mihai-sysbio
Copy link
Member

The protected branches are configured in the repository settings, so one thing would be to adjust the settings there and try the action again.

@edkerk
Copy link
Member

edkerk commented Jun 30, 2021

gh-pages is no longer a protected branch, and now the all 3 Actions succeed.
The generated history report is empty (on my computer), but is this a memote issue that should not preclude merging this PR?

@mihai-sysbio
Copy link
Member

mihai-sysbio commented Jul 1, 2021

The generated history report is empty (on my computer), but is this a memote issue that should not preclude merging this PR?

Inspecting the page shows a bunch of JavaScript errors:

ERROR Error: [data] must be specified
ERROR CONTEXT Object { view: {…}, nodeIndex: 4, nodeDef: {…}, elDef: {…}, elView: {…} }
ERROR TypeError: this.chart is undefined

As these are beyond our control, I would think it is acceptable to merge the PR just as you say @edkerk. To be on the safe side, it might make sense to run the history report locally, to see that the generated report is identical.

@mihai-sysbio mihai-sysbio self-requested a review July 1, 2021 07:30
@edkerk
Copy link
Member

edkerk commented Jul 1, 2021

Locally it produces exactly the same HTML file.

@edkerk edkerk merged commit c2d79fe into devel Jul 1, 2021
@edkerk edkerk deleted the feat/memote branch July 1, 2021 13:41
This was referenced Jul 1, 2021
@edkerk edkerk mentioned this pull request Aug 27, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new field/feature memote anything related to the memote report
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants