Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Preload extras + improvements #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Preload extras + improvements #12

wants to merge 1 commit into from

Conversation

Kacarott
Copy link
Collaborator

@Kacarott Kacarott commented Jan 28, 2024

Details of changes in this branch:

  1. Replaces the imaging script from testest with a new one in this repository, which is a more appropriate location in my opinion. (solves Move imager.factor from testest to this repository #13)
  2. The imaging script preloads core and basis as before, but also preloads a collection of useful/commonly used vocabs from extras (this solves Slow load times for extras #11)
  3. The imaging script is no longer installed as a permanent vocab (solves Do not install imager.factor as a vocabulary #14 )
  4. Removes many additional vocabs from extras which were unusable due to relying on other vocabs which were already removed. (mostly games, demos, etc) (solves Remove vocabs that rely on already removed vocabs #15)
  5. Installs math.margins from testest to work instead of making a new, prioritised vocab root pre. For 0.99 this should not change anything, but prevents breaking future versions if they add a math.margins vocab. (solves Install math.margins to work instead of making a new prioritised root "pre" #16)

I would appreciate reviews from both @kazk and @nomennescio to make sure I haven't overlooked something important.

@Kacarott Kacarott requested review from kazk and nomennescio January 28, 2024 14:59
Copy link
Collaborator

@nomennescio nomennescio left a comment

Choose a reason for hiding this comment

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

Removing additional libs from extra might be OK.
Other changes I disapprove of.

@Kacarott
Copy link
Collaborator Author

Really all other changes?? Can you at least give some reasons? After all I have good reasons for each change.

@nomennescio
Copy link
Collaborator

Really all other changes?? Can you at least give some reasons? After all I have good reasons for each change.

preloading is also probably a good idea; will look into that.

@Kacarott
Copy link
Collaborator Author

Kacarott commented Jan 29, 2024

In discord nomen has said that I should explain why I want the changes rather than him needing to provide any justification for why he doesn't like them. I answered there, but also posting here for reference:

  1. Moving imager.factor from testest to factor
  • The reason here feels obvious to me. As said above, it clearly belongs in factor, and not in testest, as it is directly involved in creating the runner instance of factor for codewars, and has nothing to do with testest. I assumed that the reason it was put in testest in the first place was simply because you previously didn't have access to the factor runner before, and therefore testest was the next best place, and so I didn't realise that this would be a point of contention.
  1. Preloading select vocabularies from extras:
  • See this issue, but you've also said that you can agree this is a problem, so I wont expand further here.
  1. Running imager.factor as a one time script instead of installing as a vocabulary:
  • It is run exactly to create the factor image and never again, and so has no purpose being a permanent vocabulary. It pollutes the vocabulary namespace, and also (very slightly) increases the size of the docker image. In general removing it makes the installation cleaner. These are fairly trivial issues, but it is also an extremely trivial fix.
  1. Removing additional broken vocabs:
  • Since a handful of vocabs were already removed due to being useless and taking up memory space, a number of others vocabs which relied on these vocabs were then broken. This change removed those, then removed vocabs which relied on this, etc. until no more vocabs relied on something broken. Not only does this save memory, it also prevents users from trying to access vocabs which appear to exist, but then break under certain circumstances.
  1. Installs math.margins to work instead of pre, including de-prioritising its loading:
  • math.margins does not need to be loaded before other vocabs, doing so adds unnecessary complexity
  • math.margins being in a prioritised root violates the factor principle of earlier loaded roots not relying on vocabs from later loaded roots. vocabs in core do not rely on vocabs in basis do not rely on vocabs in extras.
  • math.margins being prioritised means that if in the future a different math.margins vocabulary is added, then any vocabs in the monolib which rely on it will break, since they will load your math.margins instead. Causing the standard lib to break internally, is not a good thing.
  • Additionally, if a different math.margins vocabulary is added but no other library relies on it, then no error will be caused, but it will be silently shadowed, meaning that any users that try to use it in solutions will instead import the custom math.margins instead of the stdlib version.
  • If the intent is to provide an "early warning system" for future versions to inform that there is a name conflict, then adding an explicit test for that is a clearly better option, rather than relying on the standard lib potentially breaking during compilation.
  • Working around the standard factor library structure and load order is a hack and bad practice, precisely because of the bugs it could introduce

@nomennescio
Copy link
Collaborator

I object to 1, 3 and 5, because of how I see units of change related to testest and this archive, I don't want to change that.
That leaves 4 in this archive, and 2 to be addressed in testest.

@Kacarott
Copy link
Collaborator Author

Kacarott commented Jan 30, 2024

I don't know what you mean by "units of change", however there is already precedent for moving docker related things to this repository, as the Dockerfile and many other files were already moved by kazk. There is no need for imager.factor to be in testest. Moving it breaks nothing, as nothing in testest relies on it.

It doesn't make any sense that testest needs to be updated and a new release created in order to fix issues with the codewars factor image that have nothing to do with testest. Keeping it this way, when we have a clear alternative, is artificially entangling dependencies.

Note I am specifically talking about point 1 for now, as it is directly related to point 2, and makes sense to resolve them together.

@Kacarott
Copy link
Collaborator Author

About point 3, I would also like to know what benefits we gain by installing imager.factor as a vocabulary instead of running it as a one time script.

@Kacarott
Copy link
Collaborator Author

I have created issues for each change in this PR, listing the motivation behind the changes. Later I will reduce this PR to focus only on points 1, 2 and 4, leaving 3 and 5 for further discussion.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants