Problems merging doc into dev

[kslong]

There are problems merging the doc branch which Austin and Amin worked on at the 2024 Southamption meeting into dev that even after one apparently has a successful merge crop up again later trying to switch branches.

Some issues appear to be associated with the difference between LF and CRLF

Others seem to be associated with the fact that in the doc branch, it looks like the sphinx/ html directory, and various other files have been added to what is tracked.

It seems likely that the best way to proceed is to try to only add the new .rst files

[kslong]

Here is the list of files that are tracked in doc, but not tracked in dev

foo.txt

[kslong]

There are multiple problems with all of this. In the doc branch, it looks like the process of running sphinx actually does a lot more than simply create the html files. If runs various python scripts as well (to among other things get documentation from the python scripts in py_progs, and to reassess whether the parameter files) and as far as I can tell it is not documented though perhaps either @AustenWallis @Edward-RSE can direct me towards the produre. I am not convinced an integrated process is what we wanted.

[jhmatthews]

I think there are multiple things wrapped up here:

• I don't really understand the LF/CRLF thing and it seems to not be doing what was desired, so we might need to remove the git attributes stuff that @smangham added in if it's creating more issues than it's solving.
• It looks like _build was incorrectly added and gitignore isn't set up to catch this new structure (?), but I see you've removed that directory, so possible that the right files are there (but I don't know exactly what should be there with regards to the new stuff). Main thing is to make sure there are no sphinx build products at all, just the source.
• sphinx is python, it shouldn't run python scripts per se though, at least not outside of the sphinx framework. My understanding was that it should be using, e.g, the autodoc extension which means any python module with good docstrings . This is the right approach IMO, because every time you update something in py_progs it will get documented easily. Is there something I'm missing here, I'm looking in the Makefile and can only see sphinx-build commands?

[jhmatthews]

Btw, it looks like the sphinx build is failing on rtd since the last two commits to dev.

[kslong]

OK, here is a status update.

There is now a branch on the repository that contains dev, before I started mucking around. It is called before_doc_merge. So if we have to roll back to an old version of dev that is what we want.

On linux, if I download the repository as it stands from scratch. I can switch branches without any problems. (In a old local version of the repository, where I was working, I cannot do this, because of problems with kwd.png that I believe are associated with LF and CRLF)

On either the dev branch or the docs branch, I can run make html for sphinx. Errors occur of various sorts, but the sphinx is produced. The dev version of the html appears not to match the docs branch, namely it does not look like the files have been updated in the dev branch.

My ability to switch back and forth between branch does not change.

Now if I create a new branch from dev, and try to mege into it. These are the errors I see:

git merge doc
warning: Cannot merge binary files: docs/sphinx/source/images/kwd.png (HEAD vs. doc)
Auto-merging docs/sphinx/source/conf.py
CONFLICT (content): Merge conflict in docs/sphinx/source/conf.py
Auto-merging docs/sphinx/source/images/kwd.png
CONFLICT (content): Merge conflict in docs/sphinx/source/images/kwd.png
CONFLICT (rename/delete): docs/sphinx/source/input/parameters/other/Diag/Diag.turn_off_upweighting_of_simple_macro_atoms.rst renamed to docs/sphinx/source/input/parameters/other/Diag/Diag.use_upweighting_of_simple_macro_atoms.rst in HEAD, but deleted in doc.
Auto-merging docs/sphinx/source/input/parameters/other/Diag/Diag.use_upweighting_of_simple_macro_atoms.rst
CONFLICT (add/add): Merge conflict in docs/sphinx/source/input/parameters/other/Diag/Diag.use_upweighting_of_simple_macro_atoms.rst
Auto-merging docs/sphinx/source/output/spectrum_files.rst
Auto-merging docs/sphinx/source/physics/macroatoms.rst
CONFLICT (content): Merge conflict in docs/sphinx/source/physics/macroatoms.rst
Automatic merge failed; fix conflicts and then commit the result.

[kslong]

At some point though I get back to this issue, when I try to switch branches

(ksl) ~/Python/docs: git stash
warning: CRLF will be replaced by LF in docs/sphinx/source/images/kwd.png.
The file will have its original line endings in your working directory
Saved working directory and index state WIP on before_doc_merge: 8347fc54 fix incorrect comm buffer in PR #1081
(ksl) ~/Python/docs: git checkout dev
error: Your local changes to the following files would be overwritten by checkout:
    docs/sphinx/source/images/kwd.png
Please commit your changes or stash them before you switch branches.
Aborting

[kslong]

This might be the issue with the png files

https://github.com/desktop/desktop/issues/5323

[kslong]

I have made further progress on this.

• I have modified the .gitattributes file to treat .png and .jpg files as binary. This has helped with the problems with kwd.png
• I have merged doc into a branch called xdev, which I think contains most if not all of the changes made to the documentation in Southampton. This branch will merge into dev, but I have not done that except in a test environment. It will also merge into the branch before_doc_merge (which has the code before any attempts to merge docs were done.
• I have some concern that the docs branch was not fully up to date with dev, and it may have caused some back peddling in the code. and so I renamed set the version in xdev to 88c, so we can test the two versions of the code against one another.

[kslong]

I should add that make html in sphinx produces differences in the doc and xdev branch in terms of the warnings and errors you get. So this suggests there are still some problems. What needs to be done is for someone to create the sphinx documentation with the two branches and compare them page by page.

[AustenWallis]

So I have done a sphinx-autobuild on the doc branch and the xdev branch. Upon building both, the they spat out some errors.

WARNING: Title underline too short.. For Primary .pf Parameters,Others and Diagnostics, Diag.keep_ioncycle_spectra, Diag.use_upweighting_of_simple_macro_atoms, monochromatic.wavelength, Matom_transition_mode, Running Different Versions of Python and Winds. A very small fix needed but doesn't seem to affect the end documentation visually either way. Needs some extra '==='s added.

An issue that is present is in the Diag.use_upweighting_of_simple_macro_atoms.rst. This file was originally named something different and updated to reflect the new name for this option. The error is Users/austen/python/docs/sphinx/source/input/parameters/other/Diag.rst:13: WARNING: toctree contains reference to document 'input/parameters/other/Diag/Diag.use_upweighting_of_simple_macro_atoms' that doesn't have a title: no link will be generated. There is a title here, but the same error may apply with the Title underline being too short? This is being prevented from showing in the menu bullet points.

In the Wind.type.rst file, there is a broken line to Wind/models/importing_models under the imported values. This path should be updated to a correct format, or replaced with a :ref: link, similar to all other links within that file.

There is a new issue for me too with hydro_2_python.rst This is new file I am seeing. It failed to import * ModuleNotFoundError: No module named 'pyhdf'. pip install pyhdf is failing out with this error when trying to rectify this previous error note: This error originates from a subprocess, and is likely not a problem with pip. ERROR: Failed building wheel for pyhdf Failed to build pyhdf ERROR: Could not build wheels for pyhdf, which is required to install pyproject.toml-based projects

Other than that, the changes we implemented at Soton do seem to match up between the 'doc' and the 'xdev' branch. I (skimmed) compared page for page on the .rst files we noted changes with on the Evernote Documentation page. I selected some random .rst's that I remember we did minor changes too and they seem to be implemented too. The kwd.png image also appears correct on both branches for me. I have not gone through every page but a good number.

I have not implemented any of the changes for the errors described above.

[kslong]

Yes, I saw most of those errors as well. I have not tried to fix them, but do not regard them by themselves as too serious. I do think a systematic verification of that the files we want in in the sphinx source directory is needed, but writing some kind of script that checks the files one by one and locates differences. I also think that another script is needed to verify that what is in the main python source directory is what we want. I'm a bit suspicious that the merge of cod into xdev actually changed some .c files, because I am worried that somehow one of the .c files got updated in the doc branch, which should not have happened. One reson I worry about it, is that one of the regression tests has slightly different results after the update. The one that has a slightly different result is two.py, which is not as serious as if one of the others changed, but none of them should have changed.

@jhmatthews I think you should weigh in on how you want to proceed here. It seems to me the options are:

• Put the system back in the sense of getting rid of the current dev/xdev and moving the branch that has the system before any of the recent merge attempts back into the dev branch, so we keep our normal paridigm for development
• Accept that xdev us about the best we can do at this point, and make this the new dev, assuming if anything bad has happened that we will figure it out in time

I don't believe the current uncertain situation is tenable for long.

[AustenWallis]

Anything touched with .c files from the doc branch I was handling would have only been typos or incorrections in the commenting as I was following through documentation improvements. No code itself was touched.

But something that may give a hint of what might be happening ( and no clue if it is or not). There were 3 files when I sync'ed to the xdev branch this morning that said there were changes locally but not on branch. I remember setup_disk.c line 105) geo.disk_mdot /= (MSOL / YR); had THOMPSON /= (MSOL/YR) . Setup.c line 123) geo.t_bl = 100000. had THOMPSON = 100000. and there was another one , maybe bands.c line 452, It had (something) * THOMPSON instead of WIENS. The comment next to the code was something along the lines of use wiens to get or find. This is the one I don't quite remember exactly where it was.

I compared it to doc, dev and xdev and the online branches all were different to the local version so I discarded the changes locally as they were wrong. But I have no idea where they appeared from. Does your branch version have these differences too? Maybe a clue?

[AustenWallis]

Do you know which exact .c file changed from dev to doc or doc to xdev? or just that one might have?

Is dev the original correct branch at the moment? And xdev the attempted merge of doc and dev?

[kslong]

As I noted earlier, the branch before_doc_merge is the branch that is untouched by any attempts to merge docs into dev. Any differences between the .c, .h, etc, files in the Python/source directory and any others should be viewed with extreme suspicion.

[jhmatthews]

Can you talk me through this today, I'm a bit confused by this thread. Also: when I open a test PR for xdev-->dev in the main repo, it's both mergeable and doesn't change any source code - what's the drawback to just doing that PR and merge?

[smangham]

Right, apologies for the mess. The PNG files being flagged as binary thing was something I'm sure I've encountered previously and should have spotted this time at least. Argh.

sphinx is python, it shouldn't run python scripts per se though, at least not outside of the sphinx framework.

@jhmatthews Sphinx imports the scripts to generate documentation - so it can handle dependencies properly. It doesn't run them, but if the script is written such that code is executed on import, it can be a problem.

I think I added the _build/html directory so when you cloned the repo you'd have a full local copy of the documentation, but realistically that's not necessary and you can just read it on ReadTheDocs or build it yourself if need be. Removing it makes sense.

failed to import * ModuleNotFoundError: No module named 'pyhdf' ERROR: Failed building wheel for pyhdf Failed to build pyhdf ERROR: Could not build wheels for pyhdf, which is required to install pyproject.toml-based projects

@AustenWallis pyhdf needs libhdf4-dev to build, I should have added it to requirements_apt.txt. One of the reasons I included the doc HTML in the repo was so that you wouldn't need to have all the prerequisites installed to have local access, but then that only helps if the local HTML are in-date. The alternative then is either add a pre-commit hook to rebuild (...which then means every developer needs the whole of every developer's stack), or just give up on a copy in the repo and use ReadTheDocs - which is the most sensible option. Needing that local copy is such an unusual case you can prepare for it by just installing all the documentation requirements.

---

Like James I'm a little confused. doc was definitely missing commits from dev (a before_doc_merge -> doc compare is huge), but Git seems to have handled that as far as I can see. The only code changes between before_doc_merge and xdev are a typo fix in setup_star_bh.c and a blank line being added to setup.c.

[kslong]

I am going to leave his to others to resolve, but will make a few comments.

Running sphinx, e..g make html should not alter any tracked files, because that means you will get updates any time ones sphinx on different machines, and we do not want the html files, etc to be part of the repository.

The reason I was concerned about changes to the c routines is one of the regression tests, namely two.pf, showed differences when I ran a check between before_doc_merge and xdev. This may not be serious but I thought we might want to make sure we understood why that had happened.

I'm not entirely certain that the atomic data files in before_doc_merge and xdev are identical. I think xdev may have extra files, but I am not quite certain of that.

I'm largely happy with whatever you all decide to do with getting things merged. I'm not entirely certain that there were not .rst file changes in before_doc_merge that got overwritten in xdev, but even it that is the case, they are probably minor, and will easy to redo if they come up again.

My main desire is that we get back to one version of "dev" going forward, and that we confirm it can be merged into main with further problems.

[jhmatthews]

As far as I can tell, dev and before_doc_merge only differ in terms of kwd.png: https://github.com/agnwinds/python/compare/dev...before_doc_merge

and dev and xdev only differ in terms of documentation - mostly rst, some small changes to c files but not to source code: https://github.com/agnwinds/python/compare/dev...xdev

So I definitely don't think a regression test should change, and probably xdev->dev merge is all that needs doing. In future I would say the best way to manage this is to open a PR and then discuss there directly (because you can see the diffs, keep adding to the PR, and pull in changes as you go)

[kslong]

I agree that I "messed up" in the procedure I tried to use.

[jhmatthews]

Not saying that at all - do you agree things are looking OK to merge judging by those comparisons of branches?

[kslong]

I did mess up ... but yes, I agree we should merge all of this into a dev branch and continue. If a specific problem arises, we can sort it out later. I don't expect anything major.

[jhmatthews]

Ok, I've merged this with #1090. Could someone -- @AustenWallis or @kslong probably -- explain the goal of the docs/rst/ folder - is this something we should be tracking?

[AustenWallis]

Ahh yes, So docs/rst had a notebook (.ipynb) in it that made a table of .rst parameters listed in the docs, with one column being parameters on a remote branch and another local parameters on Knox's computer. This made the param_table.txt file. The missing files.py was a tiny script to list out the entries on the master table that was in one column, but not the other. Ie, highlights the missing entries that we needed to work on during the Soton week. The 'parameters/' folder seems to be a template example for Knox's script to ensure the code was working.

To me it looks like the docs/rst directory can be removed without any consequences. I also don't think it is majorly necessary for the repo.

[kslong]

We need to be careful about this, before we remove it, though obviously we can recover if we need to. The issue is how we track whether an input in python has been updated all the way to the .rst file describing the parameter. What is needed is for someone to go through the procedure and document it. That should have been me, or possibly @smangham. pr @AustenWallis I don't remember the procedure off-hand.

[jhmatthews]

Note there's a related issue here: #691

[jhmatthews]

I'm closing this since we've merged and there's already an issue on how to maintain parameter docs.