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

Optional Mutagen #749

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Optional Mutagen #749

wants to merge 3 commits into from

Conversation

joseluisi4
Copy link

This change allows you to activate or deactivate Mutagen from the warden configuration .env file with WARDEN_MUTAGEN_ENABLE=0.
By default, Mutagen is active.
A message is also painted indicating that Mutagen is being deprecated.

commands/env.cmd Outdated Show resolved Hide resolved
utils/env.sh Outdated
do
if [[ -f "${PARTIAL_PATH}" ]]; then
DOCKER_COMPOSE_ARGS+=("-f" "${PARTIAL_PATH}")
fi
done

Copy link
Member

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.

Copy link
Author

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

Copy link
Member

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

&& 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"
Copy link
Contributor

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

Copy link
Author

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

@ihor-sviziev ihor-sviziev Feb 20, 2024

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.

Copy link

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.

Copy link

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.

Copy link
Member

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.

@dudzio12
Copy link

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.

@joseluisi4
Copy link
Author

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 .env we are referring to is not the projects .env, it is a Warden configuration .env, it is in ~/.warden/.env

@dudzio12
Copy link

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 .env we are referring to is not the projects .env, it is a Warden configuration .env, it is in ~/.warden/.env

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}
Copy link
Member

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.

Copy link
Author

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"
Copy link
Member

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.

Comment on lines 96 to 97
# Set to "1" to enable Mutagen
WARDEN_MUTAGEN_ENABLE=1
Copy link
Member

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.

Suggested change
# Set to "1" to enable Mutagen
WARDEN_MUTAGEN_ENABLE=1
# Set to "1" to enable Mutagen
WARDEN_MUTAGEN_ENABLE=1

@Den4ik
Copy link

Den4ik commented Mar 1, 2024

I believe that mutagen volumes configuration should be moved to separate file, because we can have configs directly related to platform.
For example, we may use platform: ${DOCKER_PLATFORM:-linux/amd64} platform related config in MacOS. It's allow to run non ARM images on the Apple Chip. magenius-team@6ecb5eb

@dudzio12
Copy link

dudzio12 commented Oct 3, 2024

@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 :)

lewisvoncken added a commit to Happy-Horizon/warden that referenced this pull request Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📋 Backlog
Development

Successfully merging this pull request may close these issues.

7 participants