[Docs] Add a recipes page consolidating all E2E recipes#679
[Docs] Add a recipes page consolidating all E2E recipes#679SumanthRH merged 8 commits intoNovaSky-AI:mainfrom
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new documentation page to consolidate end-to-end recipes, which is a great addition for users. The implementation is mostly good, but I've identified a couple of areas for improvement in the new overview.rst file. My main feedback is around improving the consistency and maintainability of the HTML tables by consolidating CSS styles. I've also noted a potential issue with duplicated WandB links that might need correction. Overall, these are good changes that improve the documentation.
| <style> | ||
| table.skytable { | ||
| border-collapse: collapse; | ||
| margin-bottom: 20px; | ||
| } | ||
| table.skytable th, table.skytable td { | ||
| border: 1px solid #ccc; | ||
| padding: 6px 10px; | ||
| } | ||
| table.skytable th { | ||
| background: #f2f2f2; | ||
| } | ||
| table.skytable tr:nth-child(even) { | ||
| background: #fafafa; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
To improve maintainability and visual consistency across the document, I suggest consolidating the styling for all tables. Currently, there are three different styling approaches used: the skytable class, the skytable2 class, and inline styles for the eval-table. This leads to an inconsistent look and makes future updates cumbersome.
You could define a single, unified style in one <style> block at the top of the file and apply it to all tables. Here's an example:
.. raw:: html
<style>
.recipe-table {
border-collapse: collapse;
margin-bottom: 20px;
width: 100%;
}
.recipe-table th, .recipe-table td {
border: 1px solid #ccc;
padding: 8px;
text-align: left;
}
.recipe-table th {
background-color: #f2f2f2;
}
.recipe-table tr:nth-child(even) {
background-color: #fafafa;
}
.recipe-table .highlight-row {
background-color: #f2f2f2;
font-weight: bold;
}
</style>You could then:
- Apply
class="recipe-table"to all three<table>elements. - Remove the second
<style>block (forskytable2). - Remove all inline
style="..."attributes from the third table and its elements. Thehighlight-rowclass can be used for the "Average" row. - Remove the
<div style="width: 80%;">wrapper.
This would make the tables look uniform and simplify the markup significantly.
| <tr> | ||
| <td>DAPO (w/o Dynamic Sampling)</td> | ||
| <td>Qwen/Qwen-2.5-7B-Math</td> | ||
| <td>0.633</td> | ||
| <td>-0.304</td> | ||
| <td>8xH100</td> | ||
| <td>320</td> | ||
| <td><a href="https://api.wandb.ai/links/sky-posttraining-uc-berkeley/ijmo1v6q">Link</a></td> | ||
| </tr> |
There was a problem hiding this comment.
The WandB link is the same for all three entries in the DAPO recipes table. Since each row represents a different model and experiment, it's likely they should have unique links. Could you please verify that these links are correct? If they are different runs, pointing to the specific run would be more helpful for readers.
Adds a new overview page in the Recipes page consolidating all E2E recipes in SkyRL. <img width="1384" height="3160" alt="image" src="https://github.com/user-attachments/assets/55bba9ff-af63-49ca-8342-81043d1b1499" /> The table formats are a bit inconsistent, but that is fine IMO --------- Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Adds a new overview page in the Recipes page consolidating all E2E recipes in SkyRL. <img width="1384" height="3160" alt="image" src="https://github.com/user-attachments/assets/55bba9ff-af63-49ca-8342-81043d1b1499" /> The table formats are a bit inconsistent, but that is fine IMO --------- Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Adds a new overview page in the Recipes page consolidating all E2E recipes in SkyRL.
The table formats are a bit inconsistent, but that is fine IMO