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

Running diff on generated files (without detail sentences) #32720

Closed
wants to merge 10 commits into from
Closed

Running diff on generated files (without detail sentences) #32720

wants to merge 10 commits into from

Conversation

agniv-the-marker
Copy link

Description

Updated the automatic CARS.md generation to utilize wget and diff instead of pickle, which should be better (assumes less about the file structure as well). Should resolve #32690, although this does not handle "📖 Detail Sentence Changes" as this seems to not be intrinsic to the generated markdown files immediately. Would love to get some feedback on how to incorporate that here.

Verification

Simply edited the CARS.md file locally to check it on my end, not included in the commit for obvious reasons.

Copy link
Contributor

github-actions bot commented Jun 12, 2024

Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • the change is something we merge
    • include a route or your device' dongle ID if relevant

@agniv-the-marker agniv-the-marker marked this pull request as draft June 12, 2024 03:39
@agniv-the-marker
Copy link
Author

Meant to leave as a draft! Still working on the

You may need special handling of the detail sentence however, which is fine.

Sorry about that.

selfdrive/debug/print_docs_diff.py Show resolved Hide resolved
selfdrive/debug/print_docs_diff.py Outdated Show resolved Hide resolved
selfdrive/debug/print_docs_diff.py Outdated Show resolved Hide resolved
info1, info2 = line1.split('|'), line2.split('|')
return "|".join([f"{i1} {ARROW_SYMBOL} {i2}|" if i1 != i2 else f"{i1}|" for i1, i2 in zip(info1, info2, strict=True)])

def get_detail_sentence(data):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe storing the sentence as a hoverable tooltip/alt image text would be a good idea?

I'm fine with a tiny bit of parsing logic over re-creating the sentence. Remember, any changes to the website logic would then need to be mirrored here, which is very error prone

@agniv-the-marker agniv-the-marker marked this pull request as ready for review June 12, 2024 07:14
@sshane
Copy link
Contributor

sshane commented Jun 12, 2024

Can you post an example by the bot on your own fork? The result of this project should be a much cleaner and smaller docs diffing file, this grows the code.

@agniv-the-marker agniv-the-marker marked this pull request as draft June 12, 2024 15:04
@agniv-the-marker
Copy link
Author

@sshane sure! Could you quickly explain to me how to deal with the pyx files, couldn't find a solution. Running it on my codespace (using codespaces since my laptop is broken) I get:

@agniv-the-marker ➜ /workspaces/openpilot (master) $ /home/batman/pyenv/versions/3.11.4/bin/python /workspaces/openpilot/selfdrive/car/tests/test_docs.py
Traceback (most recent call last):
  File "/workspaces/openpilot/selfdrive/car/tests/test_docs.py", line 8, in <module>
    from openpilot.selfdrive.car.docs import CARS_MD_OUT, CARS_MD_TEMPLATE, generate_cars_md, get_all_car_docs
  File "/workspaces/openpilot/openpilot/selfdrive/car/docs.py", line 13, in <module>
    from openpilot.selfdrive.car.car_helpers import interfaces, get_interface_attr
  File "/workspaces/openpilot/openpilot/selfdrive/car/car_helpers.py", line 6, in <module>
    from openpilot.common.params import Params
  File "/workspaces/openpilot/openpilot/common/params.py", line 1, in <module>
    from openpilot.common.params_pyx import Params, ParamKeyType, UnknownKeyName
ModuleNotFoundError: No module named 'openpilot.common.params_pyx'

I feel like I'm maybe missing something with how you're supposed to deal with cython files? Sorry for all the comments.

Right now my bottleneck is just generating the CARS.md file which is importing this.

@sshane
Copy link
Contributor

sshane commented Jun 12, 2024

You need to build params, scons -j8 should work if you installed the openpilot deps

repo: context.repo.repo,
comment_id: ${{ steps.fc.outputs.comment-id }}
})
name: PR comments
Copy link
Contributor

Choose a reason for hiding this comment

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

don't change indentation, it makes this diff unreadable

Comment on lines +27 to +34
dump_path = os.path.join(BASEDIR, "selfdrive", "car", "tests", "cars_dump", "CARS.md")
file_path = os.path.join(dump_path, "CARS.md")
def download_file(url, filename):
import requests
response = requests.get(url)
with open(filename, 'wb') as file:
file.write(response.content)
download_file("https://raw.githubusercontent.com/commaai/openpilot/master/docs/CARS.md", file_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

this test simply checks that the file is up to date with any changes on the branch, you shouldn't need to change anything

@sshane
Copy link
Contributor

sshane commented Jun 12, 2024

CI is also failing

Copy link
Contributor

This PR has had no activity for 14 days. It will be automatically closed in 3 days if there is no activity.

@github-actions github-actions bot added the stale label Jun 27, 2024
Copy link
Contributor

This PR has been automatically closed due to inactivity. Feel free to re-open once activity resumes.

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.

Car docs bot: diff on generated readme file
2 participants