-
-
Notifications
You must be signed in to change notification settings - Fork 170
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
Conversation
utils/env.sh
Outdated
do | ||
if [[ -f "${PARTIAL_PATH}" ]]; then | ||
DOCKER_COMPOSE_ARGS+=("-f" "${PARTIAL_PATH}") | ||
fi | ||
done | ||
|
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 environment
The problem I am trying to solve is that the xxx.darwin.yml
files are not used because these volumes are designed for Mutange
Another solution would be to rename those files to xxx.mutagen.yml
, this way you could filter by name
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 is not the name of the environment. The appendEnvPartialIfExists
function takes the name of the service partial to include (e.g. nginx
, or php
), not the environment. See
Line 80 in 39d4729
&& appendEnvPartialIfExists "nginx" |
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 the WARDEN_MUTAGEN_ENABLE
variable.
commands/env.cmd
Outdated
@@ -174,7 +177,10 @@ TRAEFIK_ADDRESS="$(docker container inspect traefik \ | |||
)" | |||
export TRAEFIK_ADDRESS; | |||
|
|||
if [[ $OSTYPE =~ ^darwin ]]; then | |||
if [[ $OSTYPE =~ ^darwin ]] && [[ ${WARDEN_MUTAGEN_ENABLE} -eq 1 ]]; then | |||
echo -e "\033[31mMutagen is being deprecated!!\033[0m" |
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:
Compilation was started.
App action list generation... 8/8 [============================] 100% 11 secs 542.6 MiB
Generated code and dependency injection configuration successfully.
di:co
command execution with virtioFS:
Compilation was started.
App action list generation... 8/8 [============================] 100% 56 secs 544.6 MiB
Generated code and dependency injection configuration successfully.
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.
I have other thoughts on this topics. I have my .env files saved to git repositories and all team is using it. I have no control over environments of other people, so some might be using OrbStack, VirtioFS and some not. Since there is no global warden config, other then project env, I would suggest to implement behavior to check if mutagen is installed and run sync if it is. |
Hi @dudzio12 the |
Thanks for the clarification :) Looking forward for the warden update 👍 |
@@ -19,6 +19,9 @@ trap '' ERR | |||
## define source repository | |||
if [[ -f "${WARDEN_HOME_DIR}/.env" ]]; then | |||
eval "$(sed 's/\r$//g' < "${WARDEN_HOME_DIR}/.env" | grep "^WARDEN_")" | |||
|
|||
## configure mutagen enable by default | |||
WARDEN_MUTAGEN_ENABLE=${WARDEN_MUTAGEN_ENABLE:-1} |
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.
We should only automatically enable mutagen for Darwin OS Types. While the current code won't have an issue with this variable being enabled, in a theoretical future where we expand Mutagen support beyond Mac OS (why would we ever?) this would be enable Mutagen for people who were not expecting it.
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.
There is no problem because WARDEN_MUTAGEN_ENABLE
is not consulted by itself, to use Mutagen it is verified that it is active and that you use Darwin OS Types
commands/env.cmd
Outdated
@@ -174,7 +177,10 @@ TRAEFIK_ADDRESS="$(docker container inspect traefik \ | |||
)" | |||
export TRAEFIK_ADDRESS; | |||
|
|||
if [[ $OSTYPE =~ ^darwin ]]; then | |||
if [[ $OSTYPE =~ ^darwin ]] && [[ ${WARDEN_MUTAGEN_ENABLE} -eq 1 ]]; then | |||
echo -e "\033[31mMutagen is being deprecated!!\033[0m" |
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.
commands/install.cmd
Outdated
# Set to "1" to enable Mutagen | ||
WARDEN_MUTAGEN_ENABLE=1 |
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.
# Set to "1" to enable Mutagen | |
WARDEN_MUTAGEN_ENABLE=1 | |
# Set to "1" to enable Mutagen | |
WARDEN_MUTAGEN_ENABLE=1 |
I believe that mutagen volumes configuration should be moved to separate file, because we can have configs directly related to platform. |
@joseluisi4 Do you plan to fix issues mentioned by @navarr and @bap14? I've tested warden without mutagen for few months then upgraded warden and the speed is only minor feature for me. Lack of mutagen, which frequently causes issues with sync ex. strange symlinks, missing files etc. is much more important for me personally. If you do not have time to finish your PR, let us now, so I'll fix it and hopefully it could be released in next warden version :) |
This change allows you to activate or deactivate Mutagen from the warden configuration
.env
file withWARDEN_MUTAGEN_ENABLE=0
.By default, Mutagen is active.
A message is also painted indicating that Mutagen is being deprecated.