Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Optional Mutagen #749
base: main
Are you sure you want to change the base?
Optional Mutagen #749
Changes from 1 commit
7948a82
3d18dc8
c5f8761
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did anyone compare performance on different Docker installations, not only for OrbStack?
Unless we confirm that it works better w/o mutagen all the time, we can't make it deprecated. It will add confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ihor-sviziev, in the latest versions of Docker Desktop, performance is higher, RAM consumption is reduced and we will not have file synchronization problems.
It is important to use VirtioFS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joseluisi4 I think what @ihor-sviziev is getting at is: do we have numbers to back up the fact that Docker bind mounts are now more performant than Mutagen? Docker can spout off that it fixed memory or resource usage, but it doesn't always turn into real-world gains in every scenario.
So the question remains is do we have evidence to back up the claim that docker has higher performance, less RAM consumption and no file sync problems? Do we have it just for a single system type, or is it across all system types (Windows, Intel Mac, ARM Mac, x86_64 Linux, ARM Linux)? Without evidence to that, I think he's suggesting we maybe not mark it as deprecated yet, and wait until we get more concrete evidence that it is in fact more performant before calling it deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Particularly I remember asking someone to run these figures before and they came back with VirtioFS was not more performant than Mutagen, but unfortunately I've been having trouble finding them here or in the Den repository.
At the same time, I don't want to with-hold progress - but it would be good to know how performant VirtioFS is compared to Mutagen within large projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bap14, yeah, you're right. My suggestion was not to mark it as deprecated but rather add the ability to disable it in a backward-compatible manner.
We might add a notice like "We noticed that you're on MacOS and using Mutagen sync. We recommend configuring Docker Desktop to use VirtioFS and disable mutagen sync by adding WARDEN_MUTAGEN_DISABLED=1 to your project .env file.".
In the next six months, we can collect feedback if people see any issues by disabling mutagen on macOS. There might be cases when mutagen-enabled flow works significantly faster. If no such issues - we can finally mark it as deprecated.
Also, I have a question: how will it work for those now using mutagen but then switching to non-mutagen installation? We should probably recommend terminating some volume to reduce disk usage. This also should be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm agree with @ihor-sviziev
I have checked performance on mac M1 Pro and my subjective assessment - there is no performance difference.
Make sense to wait another developers responses and to have this option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPD
I have played little bit more with virtioFS sync and found performance degrade for cli command execution.
di:co
command execution with mutagen:di:co
command execution with virtioFS:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. @joseluisi4 please remove the deprecation notice. I fully support letting everyone make this decision for themselves, but the Warden project still fully endorses Mutagen as the best way for fast, iterative development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to look into why this section is using tabs instead of spaces, but for now please use tabs for alignment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joseluisi4 This doesn't seem like it would work to me. The value of
PARTIAL_NAME
will be "mutagen", but there's no check against the partial name.I think a better solution would be to check the
PARTIAL_NAME
prior to the for loop, and if it's "mutagen" and mutagen is disabled, then just return from the function; otherwise, process the for-loop as originally developed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PARTIAL_NAME
will never be "mutagen", it is the name of the environmentThe problem I am trying to solve is that the
xxx.darwin.yml
files are not used because these volumes are designed for MutangeAnother solution would be to rename those files to
xxx.mutagen.yml
, this way you could filter by nameThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial name is not the name of the environment. The
appendEnvPartialIfExists
function takes the name of the service partial to include (e.g.nginx
, orphp
), not the environment. Seewarden/commands/env.cmd
Line 80 in 39d4729
My reservation about removing the OS-specific partials is that if you have an os-specific partial for a service in your environment but aren't using Mutagen, it will never be included.
To me this would be an opportunity to properly refactor the mutagen inclusion to use the
appendEnvPartialIfExists
function to add Mutagen syncs, and only including them if the OS is darwin and Mutagen is enabled. This would then also mean that the checks for starting / pausing Mutagen would need to be adjusted to use theWARDEN_MUTAGEN_ENABLE
variable.