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

Remove Migrated Lecture ar1_processes #385

Merged
merged 10 commits into from
Jan 26, 2024
Merged

Remove Migrated Lecture ar1_processes #385

merged 10 commits into from
Jan 26, 2024

Conversation

HumphreyYang
Copy link
Collaborator

@HumphreyYang HumphreyYang commented Jan 24, 2024

Hi @mmcky,

This PR removes lectures that have been migrated to the intro series (QuantEcon/lecture-python-intro#334).

The AR(1) lecture is now in lecture-python-intro/pull/335

Do we still have a template for the migrated lecture? Apologize that I cannot find the template anymore in the commit thread.

@HumphreyYang HumphreyYang changed the title Remove Migrated Lectures Remove Migrated Lectures (ar1_processes and scalar_dynam) Jan 24, 2024
@mmcky
Copy link
Contributor

mmcky commented Jan 24, 2024

Do we still have a template for the migrated lecture? Apologize that I cannot find the template anymore in the commit thread.

@HumphreyYang I am not sure what you mean by template? (a process?)

We will want to also setup a redirect using sphinx-reredirect

  • setup redirect

@mmcky
Copy link
Contributor

mmcky commented Jan 24, 2024

@HumphreyYang
Copy link
Collaborator Author

Do we still have a template for the migrated lecture? Apologize that I cannot find the template anymore in the commit thread.

@HumphreyYang I am not sure what you mean by template? (a process?)

I remembered that we have a small admonition at the top that says the lecture has been migrated and will be taken down after a month. Please correct me if I were wrong.

@mmcky
Copy link
Contributor

mmcky commented Jan 24, 2024

Next Steps

Once the new locations are live and published

  • merge this PR with redirects in place for _config.yml

@HumphreyYang
Copy link
Collaborator Author

HumphreyYang commented Jan 24, 2024

Do we still have a template for the migrated lecture? Apologize that I cannot find the template anymore in the commit thread.

@HumphreyYang I am not sure what you mean by template? (a process?)

I remembered that we have a small admonition at the top that says the lecture has been migrated and will be taken down after a month. Please correct me if I were wrong.

I found it for the heavy_tails lecture. I will put this in place now.

@mmcky
Copy link
Contributor

mmcky commented Jan 24, 2024

thanks @HumphreyYang.

I guess we need to choose if we redirect automatically to the new location OR use that admonition to indicate the lecture has moved. Let's go with the admonition for now (as that is what we have done in the past) and think about the redirect process. We could add the admonition to the top of the new location (for a month instead) to say this lecture location has changed and this is the new location.

@mmcky
Copy link
Contributor

mmcky commented Jan 24, 2024

@HumphreyYang

We could add the admonition to the top of the new location (for a month instead) to say this lecture location has changed and this is the new location.

Actually - let's give this a go.

We will need to:

  • setup sphinx-reredirects (@mmcky)
  • setup the redirect for appropriate pages (@mmcky)
  • setup admonition on the new lecture (@HumphreyYang)
  • open an issue on the new lecture series to remove the admonition (@HumphreyYang)

@HumphreyYang
Copy link
Collaborator Author

HumphreyYang commented Jan 24, 2024

Many thanks @mmcky,

I think it is a better design choice. Readers cannot see the migration notice for the heavy_tail lecture as they are redirected to the new page : )

@mmcky
Copy link
Contributor

mmcky commented Jan 24, 2024

Readers cannot see the migration notice for the heavy_tail lecture as they are redirected to the new page : )

We couldn't have both as the md file can't exist when setting up the redirect. So I think the admonition in the new location is the better option. You end up in the right spot and then we let them know the location is new. I think that works well. 👍

@HumphreyYang
Copy link
Collaborator Author

scalar_dynam has been migrated (@HumphreyYang were you able to diff them to make sure all changes were transferred?)

Many thanks @mmcky,

I just ran a diff and found one section is deleted in the intro version. Perhaps we should leave it for now, and I will consult John in our next meeting.

@HumphreyYang HumphreyYang changed the title Remove Migrated Lectures (ar1_processes and scalar_dynam) Remove Migrated Lecture ar1_processes Jan 24, 2024
@HumphreyYang
Copy link
Collaborator Author

The warnings are complaining about the missing ar1_processes in the intro series.

We should get green ticks once ar1_processes is merged and live in the intro series.

@mmcky
Copy link
Contributor

mmcky commented Jan 24, 2024

@HumphreyYang this lecture is now live

https://intro.quantecon.org/ar1_processes.html

@mmcky
Copy link
Contributor

mmcky commented Jan 24, 2024

@HumphreyYang I have added the redirect for the ar1_processes lecture to https://intro.quantecon.org/ar1_processes.html

Copy link

github-actions bot commented Jan 24, 2024

@mmcky
Copy link
Contributor

mmcky commented Jan 24, 2024

@HumphreyYang well this is interesting. Are these the warnings to showed up last time?

/__w/lecture-python.myst/lecture-python.myst/lectures/finite_markov.md:677: WARNING: unknown document: intro:ar1_processes
/__w/lecture-python.myst/lecture-python.myst/lectures/inventory_dynamics.md:286: WARNING: unknown document: intro:ar1_processes
/__w/lecture-python.myst/lecture-python.myst/lectures/linear_models.md:47: WARNING: unknown document: intro:ar1_processes
/__w/lecture-python.myst/lecture-python.myst/lectures/kesten_processes.md:41: WARNING: unknown document: intro:ar1_processes
/__w/lecture-python.myst/lecture-python.myst/lectures/kesten_processes.md:173: WARNING: unknown document: intro:ar1_processes

@HumphreyYang
Copy link
Collaborator Author

Many thanks @mmcky,

I fixed them in 646cf0b

and we have green ticks after that.

Perhaps redirects overrules intersphinx? We can try deleting intersphinx tags at the front.

@mmcky
Copy link
Contributor

mmcky commented Jan 24, 2024

Many thanks @mmcky,

I fixed them in 646cf0b

and we have green ticks after that.

Perhaps redirects overrules intersphinx? We can try deleting intersphinx tags at the front.

thanks @HumphreyYang yeah there definitely seems to be an issue using both intersphinx and reredirects.

I know inter sphinx doesn't do connections to external website (at the page level) so I will take a closer look at https://documatt.com/sphinx-reredirects/usage.html to see if perhaps we should use these for internal links as well?

I am surprised though as inter sphinx is using a domain so should be independent. Will have to figure this one out.

@HumphreyYang
Copy link
Collaborator Author

HumphreyYang commented Jan 24, 2024

Many thanks @mmcky,

I experimented removing the tag or adding an empty ar1_processes but no luck.

I was also thinking about its implications on our new series. I think we can prioritize intersphinx in our new lecture series as we might be able to create an empty repository (with only the config file) hosting the redirect mappings between old intermediate series and new series. In this case, we do not have internal intersphinx references in the redirect repository, and intersphinx will also work in our new series.

@mmcky
Copy link
Contributor

mmcky commented Jan 25, 2024

I think we can prioritize intersphinx in our new lecture series as we might be able to create an empty repository (with only the config file) hosting the redirect mappings between old intermediate series and new series. In this case, we do not have internal intersphinx references in the redirect repository, and intersphinx will also work in our new series.

Thanks @HumphreyYang I don't fully understand what you are proposing. Can you explain?

We will need page redirection support to make sure anyone with python.quantecon.org/ar1_processes ends up at intro.quantecon.org/ar1_processes. This is important as 25K a month will be making use of the old addresses until they update.

@HumphreyYang
Copy link
Collaborator Author

Many thanks @mmcky

I am not sure whether it will work but I am proposing to have a fresh repository with old intermediate series URL. It will host the redirect config and intro page only (without lectures). In this case, it wouldn’t conflict with intersphinx since lectures are hosted in our target repositories.

Please let me know your thoughts on this.

@mmcky
Copy link
Contributor

mmcky commented Jan 26, 2024

@HumphreyYang are you replicating this locally -- I can't so the mystery depends. I have triggered a new build run to confirm.

@HumphreyYang
Copy link
Collaborator Author

@HumphreyYang are you replicating this locally -- I can't so the mystery depends. I have triggered a new build run to confirm.

Yes, I can replicate this locally. I will have another go today.

@mmcky
Copy link
Contributor

mmcky commented Jan 26, 2024

@HumphreyYang I am definitely getting no local build errors for either html or pdflatex using a fresh environment of the base environment.yml. It would be helpful if you can confirm. I am going to do a test on this PR by removing the reredirect setting to see if this changes -- then will restore.

@mmcky
Copy link
Contributor

mmcky commented Jan 26, 2024

@HumphreyYang the error still persists without ca5a20a so maybe intersphinx isn't working as it should?

@HumphreyYang
Copy link
Collaborator Author

Hi @mmcky,

This is getting more interesting as 646cf0b passed, and the only change afterwards was adding redirects. I am running a fresh build locally and will report my results once they are available.

@mmcky
Copy link
Contributor

mmcky commented Jan 26, 2024

@HumphreyYang I think I have figure this out 🤞 -- I think the settings are misspecified in the context of the yml file. I'll push a test commit in a minute or two.

@mmcky
Copy link
Contributor

mmcky commented Jan 26, 2024

@HumphreyYang I am rebuilding the cache to check there isn't some form of corruption.

@mmcky
Copy link
Contributor

mmcky commented Jan 26, 2024

@HumphreyYang no idea why cdf81e8 built OK when the cache hasn't finihsed rebuilding yet. Clearly some instability. I will re-run this once cache is finalised to check all is green after that.

@mmcky
Copy link
Contributor

mmcky commented Jan 26, 2024

@HumphreyYang I think this is working nicely now.

@mmcky mmcky merged commit 8a52b01 into main Jan 26, 2024
7 checks passed
@mmcky mmcky deleted the migrate-scalar-ar branch January 26, 2024 04:42
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.

2 participants