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

Workflow to check for missing keyword linking #478

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

hakonhagland
Copy link
Collaborator

See #458 (comment) for more information.

@hakonhagland hakonhagland marked this pull request as draft January 10, 2025 22:58
@hakonhagland hakonhagland force-pushed the upd_gen_kw_uri branch 2 times, most recently from f3ae7b5 to d8455b8 Compare January 13, 2025 10:11
@hakonhagland
Copy link
Collaborator Author

hakonhagland commented Jan 13, 2025

I think the workflow might be failing due to keywords with the same name appears in different chapters. For example the ADD keyword is defined in chapters 6, 7, 8, 9, and 10. The keyword-to-URI map is then not unique since the same keyword name can point to multiple URIs.

@blattms and @gdfldm How can we handle this?

@hakonhagland
Copy link
Collaborator Author

How can we handle this?

A quick-fix could be to always link to the first occurring section? So references to ADD would link to chapter 6, since that is the first occurring (when sorting 6, 7, 8, 9, and 10 numerically)

hakonhagland added a commit to hakonhagland/opm-reference-manual2 that referenced this pull request Jan 13, 2025
Reran a new version of fodt-gen-kw-uri-map (See PR OPM#478), also updated
map to include the newly added SALTMF keyword.
@hakonhagland
Copy link
Collaborator Author

This is ready for review now, so I will remove the "draft state".

@gdfldm
Copy link
Collaborator

gdfldm commented Jan 14, 2025

A quick-fix could be to always link to the first occurring section?

I think this should work for most cases. There are a few like RTEMPVD, which is in the PROPS and SOLUTION section, that is defined in the SOLUTION section.

@blattms
Copy link
Member

blattms commented Jan 16, 2025

there is a failing check. What is that about?


WARNING:root:Keyword name PLYDHFLF not found in bookmark content: PLY
WARNING:root:Keyword name PLYMAX not found in bookmark content: PLY
WARNING:root:Keyword name PLYVISC not found in bookmark content: PLY
WARNING:root:Keyword name PLYVISCS not found in bookmark content: PLY
WARNING:root:Keyword name PLYVISCT not found in bookmark content: PLY
WARNING:root:Keyword name SKIP100 not found in bookmark content: SKIP
WARNING:root:Keyword name SURFVISC not found in bookmark content: SURF
WARNING:root:Keyword name WLISTNAM not found in bookmark content: W
WARNING:root:Keyword name WPAVE not found in bookmark content: 

WARNING:root:Keyword name WSOLVENT not found in bookmark content: W
ERROR:root:Files have changed.
ERROR:root:Keyword SALTMF not found in old map.
Error: Process completed with exit code 1.

@blattms blattms requested a review from lisajulia January 16, 2025 12:21
@blattms
Copy link
Member

blattms commented Jan 16, 2025

@lisajulia Please look at the Python stuff and merge if that is ok.

@hakonhagland
Copy link
Collaborator Author

there is a failing check. What is that about?

@blattms It is this line:

poetry run fodt-gen-kw-uri-map --check-changed

I think it is nothing to worry about yet, as soon as #481 is merged, the check should pass

Reran a new version of fodt-gen-kw-uri-map (See PR OPM#478), also updated
map to include the newly added SALTMF keyword.
The latest version of poetry causes the "poetry run fodt-gen-kw-uri-map"
to fail for unknown reasons. Revert to poetry version 1.8.4 for now.
We need to reset it at the end of the paragraph not at
the beginning of the next tag, since the table caption may
include tags like text:s
Add an option --all that will run the keyword linker script on
all files.
Added --check-changed option to the keyword linking script. If this
option is given the script will not update any files but just check
if any of the files would be changed by running the script. It will
exit with code 1 if any file would be changed.
Run the script fodt-link-keywords --check-changed from the
workflow to prevent PRs that would change the keyword linking from
being merged.
If "fodt-gen-kw-uri-map --check-changed" detects a change, also print
information about what changed. This can be helpful for debugging.
Since a keyword may defined in multiple chapters, e.g. ADD which is
defined in chapters 6.3, 7.3, 8.3, 9.3, and 10.3, it is important to
always link to the same chapter. To achieve this, we will always link
to the first occuring definition. "First" here means to the chapter
we the smalles numerical value. So ADD would here link to the definition
in chapter 6.3 since 6.3 is smaller than 7.3, 8.3, 9.3 and 10.3.
Previous commits reverted the keyword_uri_map_generator.py file
by mistake to a previous version.
Deactivate the second check until links have been updated in the
chapters and we can expect the check to pass
scripts/python/src/fodt/keyword_linker.py Outdated Show resolved Hide resolved
# fodt-gen-kw-uri-map --maindir=<main_dir> --keyword_dir=<keyword_dir>
# fodt-gen-kw-uri-map --maindir=<main_dir> \
# --keyword_dir=<keyword_dir> \
# --check-changed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, I ran fodt-link-keywords --check-changed it said:
INFO:root:Files have not changed.
but fodt-gen-kw-uri-map changed said:
INFO:root:Generated keyword URI map to /home/lnebel/git_repos/opm/opm-reference-manual/scripts/python/../../parts/meta/kw_uri_map.txt

Copy link
Collaborator Author

@hakonhagland hakonhagland Jan 17, 2025

Choose a reason for hiding this comment

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

I guess this is because fodt-link-keywords --check-changed in this PR does not include the commit in #482 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah sorry I just saw I was actually using the branch haha/upd_link_kw2

Clarify that fodt-link-keywords will not actually save the generated
map to file.
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.

4 participants