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

Clarify package (re)loading use cases #1974

Closed
Tracked by #2044
alerque opened this issue Jan 28, 2024 · 14 comments
Closed
Tracked by #2044

Clarify package (re)loading use cases #1974

alerque opened this issue Jan 28, 2024 · 14 comments
Labels
documentation Documentation bug or improvement issue question Ask for advice or investigate solutions todo
Milestone

Comments

@alerque
Copy link
Member

alerque commented Jan 28, 2024

A possible negative impact of the change is that the multiple package instantiation model introduced in SILE 0.13-0.14 may cause some havoc in some workflows not using a resilient class.

Originally posted by @Omikhleia in #1866 (comment)


I seem to have lost track about what the perceived the problem is with the current model. I know there were some hiccups even with core packages when we first changed up the loader, but I thought the issues were ironed out. Packages are allowed to accept initialization options, so blocking the initialization would block the most obvious user facing way to re-initialize with new options. As it stands all the core packages are idempotent (or something close) and this hasn't seemed to be an issue yet.

As for 3rd party packages that want to make a different choice and only allow loading once, I don't lee what is to stop them using the current model.

local _initialized

function package:_init ()
    if _initialized then return self end
    ...
    _initialized = true
end

What is there left that is not covered by the current model of allowing re-initialization (either to correct things clobbered by other packages or use new options) and also allowing the package itself to do whatever it wants as far as whether to to actually do anything on initialization?

@alerque alerque added the question Ask for advice or investigate solutions label Jan 28, 2024
@alerque
Copy link
Member Author

alerque commented Jan 28, 2024

One of the early issues was that each instance of \use would cause the Lua source to be re-loaded and the existing instantiation of a package to be orphaned. This did leave some cruft around in memory and potentially ruin the shimming process. Also it made it so that a module could not readily tell if it had even been instantiated.

I believe that issue has been addressed. Consider for example:

\begin{document}
\use[module=packages.lorem]
\lua{print(SILE.packages["lorem"])}
\use[module=packages.lorem]
\lua{print(SILE.packages["lorem"])}
\end{document}
$ ./sile repack.sil
SILE v0.14.14.r303-ga8f6166 (LuaJIT 2.1.1700206165) [Rust]
<repack.sil> as sil
<./core/sile.lua:444> as lua
table: 0x55d29d722590
<./core/sile.lua:444> as lua
table: 0x55d29d722590
[1]

Note the table ID is the same even after reloading.

Is the issue that we need to also stash the actual Penlight class instances for each module? It does appear that we are not doing that (I actually thought we were but couldn't find it right now).

@Omikhleia
Copy link
Member

Omikhleia commented Jan 28, 2024

Script:

\begin{document}
\lua{
print("1a. In SILE packages (class)")
for k, v in pairs(SILE.packages) do
  print(k,v)
end
print("1b. In document class (instances)")
class = SILE.documentState.documentClass
for k, v in pairs(class.packages) do
  print(k, v)
end
}

\use[module=packages.url]
\lua{
print("2a. In SILE packages (class)")
for k, v in pairs(SILE.packages) do
  print(k,v)
end
print("2b. In document class (instances)")
class = SILE.documentState.documentClass
for k, v in pairs(class.packages) do
  print(k, v)
end
}

\use[module=packages.verbatim]
\lua{
print("3a. In SILE packages (class)")
for k, v in pairs(SILE.packages) do
  print(k,v)
end
print("3b. In document class (instances)")
class = SILE.documentState.documentClass
for k, v in pairs(class.packages) do
  print(k, v)
end
}
\use[module=packages.url]
\lua{
print("4a. In SILE packages (class)")
for k, v in pairs(SILE.packages) do
  print(k,v)
end
print("4b. In document class (instances)")
class = SILE.documentState.documentClass
for k, v in pairs(class.packages) do
  print(k, v)
end
}
\end{document}

Commented output from SILE v0.14.16 (LuaJIT 2.1.1702233742)

(1. Initial state)

1a. In SILE packages (class) => EMPTY

1b. In document class (instances)
raiselower	...
footnotes	...
infonode	...
leaders	...
insertions	...
counters	...
masters	...
twoside	...
bidi	...
folio	...
tableofcontents	...

(2. After \using url)

2a. In SILE packages (class) 
url	table: 0x7f9ca2d651b8

2b. In document class (instances)
bidi	...
inputfilter	inputfilter: 0x7f9ca2d6f578 => LOADED BY URL
raiselower	...
verbatim	verbatim: 0x7f9ca2d6a5c8 => LOADED BY URL
footnotes	...
infonode	...
leaders	...
insertions	...
counters	...
masters	...
twoside	...
pdf	pdf: 0x7f9ca2d79cc0 => LOADED BY URL
folio	...
tableofcontents	...

(3. After using \verbatim)

3a. In SILE packages (class)
url	table: 0x7f9ca2d651b8
verbatim	table: 0x7f9ca2d6a3d0
3b. In document class (instances)
bidi	...
inputfilter	inputfilter: 0x7f9ca2d6f578
raiselower	...
verbatim	verbatim: 0x7f9ca2d6a5c8 => UNCHANGED
footnotes	...
infonode	...
leaders	...
insertions	...
counters	...
masters	...
twoside	...
pdf	pdf: 0x7f9ca2d79cc0 => UNCHANGED
folio	...
tableofcontents	...

(4. After \using url)
4a. In SILE packages (class)
url	table: 0x7f9ca2d651b8
verbatim	table: 0x7f9ca2d6a3d0
2b. In document class (instances)
bidi	...
inputfilter	inputfilter: 0x7f9ca2d7ffa0 => NEW INSTANCE
raiselower	...
verbatim	verbatim: 0x7f9ca2d7f568 => NEW INSTANCE
footnotes	...
infonode	...
leaders	...
insertions	...
counters	...
masters	...
twoside	...
pdf	pdf: 0x7f9ca2d80588 => NEW INSTANCE
folio	...
tableofcontents	...

None of this seems logical to me:

  • SILE.packages doesn't contain the package classes loaded from other paths than \use
  • Packages loaded by \use are not seen as class-loaded, but their dependencies are.
  • Some of these dependencies of the \used one are re-instantiated (and itself is too likely).

@Omikhleia
Copy link
Member

By the way your example above is slightly misleading:

\begin{document}
\lua{print(SILE.packages["lorem"])}% This does actually load the package,
                                   % even without prior `\use` (but doesn't instantiate it)
\end{document}
SILE v0.14.16 (LuaJIT 2.1.1702233742)
<temp/pack.sil> as sil
</usr/share/sile/core/sile.lua:444> as lua
table: 0x7f61d47c27b8

@Omikhleia
Copy link
Member

Omikhleia commented Jan 28, 2024

And a "reasonable" use case.

\begin{document}
% In my preamble...
\use[module=packages.url]
\define[command=urlstyle]{<\process>}% My style for this book.

% Later in some included document...
See \url{https://sile-typesetter.org/}

% Much later in some other included document, also used in another book...
\use[module=packages.url]
See also \url{https://sile-typesetter.org/}

\end{document}

image

Do you really think this is what people expect?
Make it worse, the second \use[module=packages.url] could be indirect (i.e. loading another packages, that wants to ensure "url" is loaded as it will use it in its own commands).

The question is not related to my packages (whether they'd hack a if _initialized then return self end or whatever for their own sake). I'm only using core packages in this example, and they get re-instantiated multiple times, and overrides got cancelled, just because one typist included something apparently innocuous in a deeply nested sub-document. And this gets past that included file, and affects the rest of the document...

You may say it's per design. I might say this is a wrong and broken paradigm.

@Omikhleia
Copy link
Member

Omikhleia commented Jan 29, 2024

Make it worse (...) indirect (...)

\begin{document}
\use[module=packages.url]
\define[command=urlstyle]{<\process>}% My style for this book.
See \url{https://sile-typesetter.org/}

\use[module=packages.markdown]
\begin[type=markdown]{raw}
Hello from _Markdown_.
\end{raw}
\par% Unrelated crust.

See \url{https://sile-typesetter.org/}
\end{document}

Oops:
image

= "markdown" internally loads "url" (via self:loadPackage("url") in some init) just to ensure \href is available. (It doesn't even use \url (yet)).

@alerque
Copy link
Member Author

alerque commented Jan 30, 2024

Thanks for the feedback. There are some points here that take me by surprise because that is not what I thought was going on, and also a couple points that I disagree with (e.g. yes in some cases I expect the results you suggest are unexpected), but either way this gives me something to work through.

@alerque alerque added this to the v0.15.0 milestone Jan 30, 2024
@alerque alerque added documentation Documentation bug or improvement issue todo labels Jan 30, 2024
@alerque
Copy link
Member Author

alerque commented Jan 30, 2024

One major issue I just uncovered is that class:loadPackage() (which I use extensively) is handling instantiation differently than sile -u, SILE.use() or \use. Hence why I haven't noticed some issues with the latter. The former is storing instantiated copies of packages, the latter is storing the package class not the instance.

@alerque
Copy link
Member Author

alerque commented Jan 30, 2024

@Omikhleia Have you used SILE develop at all since #1908? It looks like I need to apply that stuff to SILE.use() is it gets picked up elsewhere too, but at a guess will those changes to instantiation cover your usage expectations?

@Omikhleia
Copy link
Member

How can this be tagged as a "documentation" (only) issue, since:

There are some points here that take me by surprise because that is not what I thought was going on

I had problem with this since July 2022 (#1366 (comment)), which was unanswered

I then opened a dedicated issue in August 2022, #1531 which got closed though the above-mentioned concerns were unaddressed.

I was bugged here in Dec 2022, and mentioned the multiple instantiation problem again in my feedback.

I reported numerous related issues (let's pick just one, February 2023, #1708)... That were fixed without concern for the global re-instantiation picture.

Honestly, my sile·x stuff was mostly created in Jul 30, 2023 because of this very pain point, as I wanted to remove hacks from other components. Sure, it then received other stuff (either in-advance experiments or backported PRs), but for everything new I've tried to play fair and also propose upstream updates too when I felt them to be ready. The multiple re-instantiation issue was the initial forking point.

I was bugged again in Nov. 2023 -- though it does not fix anything...

I was yet bugged again in Nov. 2023 for the very way sile·x worked -- and I addressed it, though it implies other workflows as described above might be affected.

Frankly, I don't know what you expect me to do here -- I've said numerous times I was ill-at-ease with this behavior and could not find a way to sort it out.

@Omikhleia
Copy link
Member

Erm, re-reading my previous answer, it might sound harsh, which wasn't the intent.
I'm really at loss on the topic (for a long time indeed, but that's life). I don't even have any subtle idea what would be 'the couple points" where you would disagree based on the above scenarios, and what are the good reasons for this (but if it's for the very few packages that truly have init options, they might be an edge-case which could have been addressed differently without re-instantiation)

@alerque
Copy link
Member Author

alerque commented Jan 31, 2024

How can this be tagged as a "documentation" (only) issue, since:

It isn't. I also tagged it todo . As in it isn't a bug Software bug issue or enhancement Software improvement or feature request but does require some code work. At this point at the very least fixing the use loader to work the same as the class method, and probably more to protect the existing instances if present. At the same time it also needs documentation Documentation bug or improvement issue and also is an open question Ask for advice or investigate solutions because I didn't (maybe don't) fully understand the use cases.

@alerque
Copy link
Member Author

alerque commented Jan 31, 2024

Frankly, I don't know what you expect me to do here

At this point the main gist of my expectation is in regard to this comment: Does the behavior in the develop branch slated to become v0.15.0 address your concerns or not? Your previous issue #1531 was closed by #1908, but your comments in this issue are evidently from the 0.14.x release series and still showing the old behavior. For example this MWE:

And a "reasonable" use case.

\begin{document}
% In my preamble...
\use[module=packages.url]
\define[command=urlstyle]{<\process>}% My style for this book.

% Later in some included document...
See \url{https://sile-typesetter.org/}

% Much later in some other included document, also used in another book...
\use[module=packages.url]
See also \url{https://sile-typesetter.org/}

\end{document}

image

This is not the result you should be getting with the develop branch. I get this:

20240131_14h09m27s_grim

While for this particular sample it gives the same results, for more advanced cases #1979 takes things a step farther and makes the new behavior more robust to allow reloads of modules to load the same instance as the original load (so that anything in the package's self is not lost even when explicitly asking for a reload that resets commands and settings).

@Omikhleia
Copy link
Member

Omikhleia commented Feb 3, 2024

Does the behavior in the develop branch slated to become v0.15.0 address your concerns or not?

Quick answer:
I don't know yet - I am slowly getting there, but I haven't tried the develop branch much. All my above examples were on (as-of-now current) SILE 0.14.16.
(My production workflow (for both the book I made in Nov. 2023 as well as the book I'm currently typesetting) is still in on a custom 0.14.11 build with many patches, and not even using my own latest "resilient" modules ^^ -- My development workflow just migrated from 0.14.14 to 0.14.16 (skipping all other versions in between), with no hiccups so far, so I plan to update my production workflow, but I am very cautious here.)

@alerque
Copy link
Member Author

alerque commented May 30, 2024

@Omikhleia Did you ever get a chance to test the develop branch in relation to package reloading to see if it actually addresses your usage concerns like I think it does?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation bug or improvement issue question Ask for advice or investigate solutions todo
Projects
None yet
Development

No branches or pull requests

2 participants