-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
Fixed user and sudo under docker #495
Conversation
@alanwilter, to be clear, this fixes issues introduced with #464, correct? Could you please detail a bit more the kind of issues that were introduced by this? For us to understand the changes, for other users that may face the same problems… thanks. |
amlb/runners/docker.py
Outdated
self.minimize_instances = rconfig().docker.minimize_instances | ||
self.container_name = 'docker' | ||
self.container_name = "docker" |
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.
nitpick: thanks for not reformatting the entire file when you apply changes. For example, for strings I tend to use single quotes for keys/constants and double quotes for display strings: for my usage, it improves readibility. But more importantly, this PR/diff would focus only on relevant changes.
Same with new lines and so on. So, please revert all formatting changes, as I'd prefer not to see this becoming a new habit, thank you.
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.
Sorry about that, my setup is with black, pyupgrade, pre-commit, isort etc. I will disable for this project.
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.
@alanwilter, to be clear, this fixes issues introduced with #464, correct?
Could you please detail a bit more the kind of issues that were introduced by this? For us to understand the changes, for other users that may face the same problems… thanks.
Yes, I found out that when running docker and docker line RUN $PY {script} {framework} -s only
was called, some frameworks need to install ubuntu packages etc. and this was failing.
Now all runs as the local user
issuing the docker cmd and will save all for that user
, and this user
is enabled to run sudo
inside the docker, without password.
The other issue addressed was that, again, when docker running RUN $PY {script} {framework} -s only
, for some frameworks, directive ENV DEBIAN_FRONTEND noninteractive
was not working to avoid tzdata
to enter in the interactive mode. So, I found better to install it at the first opportunity. You can rearrange it as you wish.
I found this issue when debugging autoxgboost
, which installs
SUDO apt-get install -y r-base r-base-dev
SUDO apt-get install -y libgdal-dev libproj-dev
SUDO apt-get install -y libssl-dev libcurl4-openssl-dev
SUDO apt-get install -y libcairo2-dev libudunits2-dev
and one of these pkgs has tzdata
dependency.
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.
looks good to me, thanks for addressing this @alanwilter: mainly concerned about the behaviour of os.getXXid()
on Windows.
Waiting for feedback from @PGijsbers about this, then I think we can merge.
amlb/runners/docker.py
Outdated
@@ -54,7 +57,7 @@ def _start_container(self, script_params=""): | |||
inst_name = f"{self.sid}.{str_sanitize(str_digest(script_params))}" | |||
cmd = ( | |||
"docker run --name {name} {options} " | |||
'-u "$(id -u $USER):$(id -g $USER)" ' | |||
f'-u "{self.userid}:{self.usergid}" ' |
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.
for Windows users, docker mode is often the only mode available to run most of the frameworks, so we need to ensure that this still works on Win.
@PGijsbers I now wonder how the original $(id -u $USER)
calls worked on Win, are we requiring Bash on Windows? In this case, how does the Py commands os.getuid()
and os.getgid()
behave on Win?
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 can confirm this breaks on my windows system. This matches the Python documentation that availability is limited to Unix for those two commands.
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 can't test that, but Windows (and Mac), don't mess up root with user, i.e., you run a docker as root that saves files, those files will belong to original windows/mac user and not to the root, like it happens in Linux.
Perhaps, just wrap these lines with a if linux
...
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.
Seems reasonable to me since it's a platform specific issue.
@alanwilter |
Thanks @alanwilter for fixing the initial problem! |
You're welcome. I still need to find time to check the out #495 (comment) |
Since there was no response for 3 weeks, I wasn't sure if you were still interested in trying so I went ahead and merged. If you do ever find the time to check, we would appreciate a quick confirmation :) But there's no rush, we'll operate under the assumption the problem is fixed (because, as far as I can tell, it is). |
This introduced an issue when building public images: the image is set up for the builder, but the user will start the container under a different uid, which leads to permission errors. I think we should maybe remove any user information from the docker container, and instead create/assign permissions on startup with something like: set -e
if [ $UID = 0 ]; then
echo "Docker started as root, not changing file permissions."
exit 0
fi
user_id=$UID
echo "root" | su -c "adduser --disabled-password --gecos '' -uid $user_id amlb"
echo "root" | su -c "echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers"
echo "root" | su -c "chown -R amlb:amlb /home/amlb"
echo "root" | su -c "hown -R amlb:amlb /bench"
echo "root" | su -c "passwd -d amlb"
su "amlb" |
There's probably cleanier ways to set this up (though whatever UID is inserted is unlikely to be in de sudo file), but my experience lacks here. |
@alanwilter something like that should still solve your issue, right? |
* Add a workflow to tag latest `v*` release as `stable` (#399) Currenty limited to alphabetical ordering which means that any one number in the version can not exceed one digit. * Bump auto-sklearn to 0.14.0 (#400) * Update version to 2.0 * Revert "Update version to 2.0" This reverts commit 9e0791a. * Fix/docker tag (#404) * Add the version tag to the image name if present * Fix casing for MLNet framework definition * Sync stable-v2 and master (#407) * Update version to 2.0.2 * Revert version change * Add support for the OpenML test server (#423) * Add support for the OpenML test server * change domain from openmltestserver to test.openml * update error message * Apply suggestions from code review Co-authored-by: seb. <[email protected]> * fix syntax error due to online merging Co-authored-by: seb. <[email protected]> * Switch from release:created to release:published (#429) * Added support for dataset files stored on s3 (#420) * s3 functionality * Update amlb/datasets/fileutils.py Co-authored-by: Pieter Gijsbers <[email protected]> * OOD * add s3n * move boto3 import Co-authored-by: Weisu Yin <[email protected]> Co-authored-by: Pieter Gijsbers <[email protected]> * Respect TMP, TMPDIR, TEMP (#442) * Respect tmpdir * Fixed submodule * feat: retain environment vars for framework venv * minor fix on compatibility (#454) Co-authored-by: Qingyun Wu <[email protected]> * Ignore decoding errors on Windows (#459) By default it can use cp1252 decoding which sometimes raises an error and halts the process. * Fix a typo (#462) will used -> will be used * Merge back stable-v2 to master (#472) * Add `stable` tag workflow, bump auto-sklearn (#401) * Add a workflow to tag latest `v*` release as `stable` (#399) Currenty limited to alphabetical ordering which means that any one number in the version can not exceed one digit. * Bump auto-sklearn to 0.14.0 (#400) * Fix/docker tag (#404) * Add the version tag to the image name if present * Fix casing for MLNet framework definition * Changed latest from master to main * Update version to 2.0.1 * Improv/aws meta (#413) * Add volume meta data to aws meta info * Add constraints for v2 benchmark (#415) * Add constraints for v2 benchmark For ease of reproducibility, we want to include our experimental setup in the constraints file. For our experiments we increase the volume size to 100gb and require gp3 volumes (general purpose SSD). * Update version to 2.0.2 * Fix AWS random cancel issue (#422) * let the job runner handle the rescheduling logic to ensure that the job is always can't be acted upon by current worker after being rescheduled * remove commented code * Add a GAMA configuration intended for benchmarking (#426) Made the previous version abstract to avoid accidentally running the wrong version of GAMA for the benchmark. * Unsparsify target variables for (Tuned)RF (#425) * Unsparsify target variables for (Tuned)RF Sparse targets are not supported in scikit-learn 0.24.2, and are used with tasks 360932 and 360933 (QSAR) in the benchmark. * cosmetic change to make de/serialization easier to debug Co-authored-by: Sebastien Poirier <[email protected]> * ensure that openml is configured when loading the tasks (#427) * Expect a possible `NoSuchProcess` error (#428) Since it's entirely possible that the processes were already terminating, but only completed termination between the process.children call and the proc.terminate/kill calls. * Reset version for versioning workflow * Update version to 2.0.3 * ensure that the docker images can be built from linux (#437) * Avoid querying terminated instance with CloudWatch (#438) * fixes #432 add precision to runtimes in results.csv (#433) * fixes #432 add precision to runtimes in results.csv * Update amlb/results.py Co-authored-by: seb. <[email protected]> Co-authored-by: seb. <[email protected]> * Iteratively build the forest to honor constraints (#439) * Iteratively build the forest to honor constraints In particular depending on the dataset size either memory or time constraints can become a problem which makes it unreliable as a baseline. Gradually growing the forest sidesteps both issues. * Make iterative fit default, parameterize execution * Step_size as script parameter, safer check if done When final_forest_size is not an exact multiple of step_size, randomforest should still terminate. Additionally step_size is escaped with an underscore as it is not a RandomForestEstimator hyperparameter. * Iterative fit for TunedRandomForest to meet memory and time constraints (#441) * Iterative fit to meet memory and time constraints Specifically for each value of `max_features` to try, an equal time budget is alloted, with one additional budget being reserved for the final fit. This does mean that different `max_features` can lead to different number of trees, but it keeps it simple. * Abort tuning when close to total time budget The first fit of each iterative fit for a `max_features` value was not guarded, which can lead to exceeding the total time budget. This adds a check before the first fit to estimate whether the budget will be exceeded, and if so aborting further tuning and continue with the final fit. * Make k_folds configurable * Add scikit-learn code with explanation * Modify cross_validate, allow 1 estimator per split This is useful when we maintain a warm_started model for each individual split. * Use custom cv function to allow warm-start By default estimators are cloned in any scikit-learn cross_validate function (which stops warm-start) and it is not possible to specify a specific estimator-object per fold (which stops warm-start). The added custom_validate module makes changes to the scikit-learn code to allow warm-starting to work in conjunction with the cross-validate functionality. For more info see scikit-learn#22044 and scikit-learn#22087. * Add parameter to set tune time, rest is for fit The previous iteration where the final fit was treated as an equivalent budget to any other optimization sometimes left too little time to train the final forest, in particular when the last fit took longer than expected. This would often lead to very small forests for the final model. The new system guarantees roughly 10% of budget for the final forest, guaranteeing a better final fit. * Revert version to _dev_version to prepare release (#444) * Update version to 2.0.4 * Signal to encode predictions as proba now works (#447) In a previous iteration it was encoded as a numpy file, but now it's serialized to JSON which means that results.probabilities is simply a string if imputation is required. * Monkeypatch openml to keep whitespace in features (#446) Technically monkeypatch xmltodict function used by openml when reading the features xml * fixe for mlr3automl (#443) * Reset version for Github workflow (#448) * Update version to 2.0.5 * Update mlr3automl to latest Was supposed to be included with #443 * Update MLR3 (#461) * Reset version for version bump * Updatet version because GA failed * Issue 416: fixing versioning workflow for releases and merges to master (#468) * change workflow to correctly modify the app version on releases and when forcing merged version back to master * protect main branch from accidental releases * fix stress test Co-authored-by: PGijsbers <[email protected]> Co-authored-by: eddiebergman <[email protected]> Co-authored-by: github-actions <[email protected]> Co-authored-by: Erin LeDell <[email protected]> Co-authored-by: Stefan Coors <[email protected]> * useless workflow reintroduced during merge (#475) * tag all AWS entities (#469) * fixed parsing of int targets when loading file in CSV format (#467) * Avoid root owned files from docker (#464) * New site (#479) * First draft of new website * Add framework descriptions, papers and logos * Update footer with Github link * Remove under construction banner * Add redirect from old page to new one * Update page title * Add text links to new paper to be added later * Move static site to /docs * Whitelist documentation images * Remove temporary work directory * Add documentation images * Place holder for mobile * Move old notebooks and visualizations To make sure they are not confusing for new users, as these will no longer work out-of-the-box. New notebooks will be added soon but I don't have the files available right now. * Tell github this is not Jekyll * Update minimal responsiveness (#480) * Make results responsive (hacky) * Make Frameworks page more responsive * Make Home more responsive * Bare minimum mobile navbar * Make sure phones report fake width * Link to arxiv paper (#481) * Update to support AutoGluon v0.4 (#455) * Update to support AutoGluon v0.4 * Address comments * Updated setup.py for `hyperoptsklearn` as it no longer uses PyPi (also now accepts shas) (#410) * Updated hyper opt not to use PyPi and accept shas * case-sensitive PIP command in setup Co-authored-by: Sebastien Poirier <[email protected]> * AutoGluon TimeSeries Support (first version) (#494) * Add AutoGluon TimeSeries Prototype * AutoMLBenchmark TimeSeries Prototype. (#6) * fixed loading test & train, changed pred.-l. 5->30 * ignore launch.json of vscode * ensuring timestamp parsing * pass config, save pred, add results * remove unused code * add readability, remove slice from timer * ensure autogluonts has required info * add comments for readability * setting defaults for timeseries task * remove outer context manipulation * corrected spelling error for quantiles * adding mape, correct available metrics * beautify config options * fixed config for public access * Update readme * Autogluon timeseries, addressed comments by sebhrusen (#7) * fixed loading test & train, changed pred.-l. 5->30 * ignore launch.json of vscode * ensuring timestamp parsing * pass config, save pred, add results * remove unused code * add readability, remove slice from timer * ensure autogluonts has required info * add comments for readability * setting defaults for timeseries task * remove outer context manipulation * corrected spelling error for quantiles * adding mape, correct available metrics * beautify config options * fixed config for public access * no outer context manipulation, add dataset subdir * add more datasets * include error raising for too large pred. length. * mergin AutoGluonTS framework folder into AutoGluon * renaming ts.yaml to timeseries.yaml, plus ext. * removing presets, correct latest config for AGTS * move dataset timeseries ext to datasets/file.py * dont bypass test mode * move quantiles and y_past_period_error to opt_cols * remove whitespaces * deleting merge artifacts * delete merge artifacts * renaming prediction_length to forecast_range_in_steps * use public dataset, reduced range to maximum * fix format string works * fix key error bug, remove magic time limit * Addressed minor comments, and fixed version call for tabular and timeseries modularities (#8) * fixed loading test & train, changed pred.-l. 5->30 * ignore launch.json of vscode * ensuring timestamp parsing * pass config, save pred, add results * remove unused code * add readability, remove slice from timer * ensure autogluonts has required info * add comments for readability * setting defaults for timeseries task * remove outer context manipulation * corrected spelling error for quantiles * adding mape, correct available metrics * beautify config options * fixed config for public access * no outer context manipulation, add dataset subdir * add more datasets * include error raising for too large pred. length. * mergin AutoGluonTS framework folder into AutoGluon * renaming ts.yaml to timeseries.yaml, plus ext. * removing presets, correct latest config for AGTS * move dataset timeseries ext to datasets/file.py * dont bypass test mode * move quantiles and y_past_period_error to opt_cols * remove whitespaces * deleting merge artifacts * delete merge artifacts * renaming prediction_length to forecast_range_in_steps * use public dataset, reduced range to maximum * fix format string works * fix key error bug, remove magic time limit * swapped timeseries and tabular to set version * make warning message more explicit * remove outer context manipulation * split timeseries / tabular into functions Co-authored-by: Leo <[email protected]> * Add workflow to manually run `runbenchmark.py` on Github Actions (#516) * Add workflow for manually running a test benchmark * Use built-in context for getting the branch * Add more info to step names * Add ability to specify options * Fixed user and sudo under docker (#495) * Fixed user and sudo under docker * Reverted format * Update docker.py * Addressing #497 #497 * Keep wget quiet * Use :, . is deprecated Co-authored-by: seb. <[email protected]> * Set username and userid in Dockerfile generation * Install HDF5 to Docker for tables * Avoid using unix-specific workarounds on Windows * Re-enable caching for building docker images --------- Co-authored-by: seb. <[email protected]> Co-authored-by: PGijsbers <[email protected]> * [no-ci] Fix broken link (#514) * Remove autoxgboost, add `removed` field for frameworks (#519) * Add redirect for dataset page (#521) * Upgrade Python version and dependencies (#520) * Remove usage of np.float alias and just use float * Bump to Py3.9 * Update requirements for March 2023, Py3.9 * Pin packaging, since LegacyVersion was removed. Also remove scipy pin, since later autosklearn needs higher scipy. * Install packages to ranger/lib * Set secret PAT used when installing with R remotes Specifically for mlr3automl integration * Update usage for oct 21 release * Disable custom installed packages * Remove installation of reqiurements altogether * Insert oboe example * Add monkeypatch * Make error matrix numpy array * Upgrade to Ubuntu 22.04 from 18.04 * Update pip cache to look at 3.9 directory * Add Github PAT to run_all_frameworks script * bump github action versions * Adding tarfile member sanitization to extractall() (#508) * Included lightautoml in frameworks_stable (#412) * Included lightautoml in frameworks_stable * Added MLNet to frameworks_latest * Added mlr3 to both stable and latest * copy/paste fix * Remove travis file (#529) * Remove travis file since it is not used * Update readme to reflect Python 3.9 support * Add github action workflow to replace old travis file * Add job id, improve name * Fix bug where task inference would lead to KeyError * Update type data for new openml/pandas Probably ought to remove the specific check if we don't enforce it. * Write numeric categories as str, see renatopp/liac-arff/issues/126 * [Open for review] Store results after each job completion (#526) * ensure that results are solved progressively in all situations instead of only when all jobs are completed * rename config flag * don't forget to cleanup job runner exec thread * Improve type hints * Adding file lock on global results file (#453) * adding file lock on global results file * fix imports * fix amlb.utils export * cosmetic * clranup util imports (also magic strings) + remove ruamel dependency in subprocesses --------- Co-authored-by: Sebastien Poirier <[email protected]> * Update the requirements files to exclude yaml and include filelock The remainder of dependencies are not re-generated to avoid additional changes in the PR. * Add missing import * Add fallback for when job is not started * Return an empty dataframe if dataframe is empty This avoids a bug where an empty dataframe is indexed. * Inform the user result summary is not available in AWS mode As results are processed in a different manner (files are directly copied over from S3). This avoids a bug where a benchmark results.csv file tries to be accessed. * Separate scoreboard generation to two lines instead Which makes it easier to tell which part of the generation generates an error, if any. * re-enable logging * Provide a warning and return early if no process output is detected This avoids potentially crashing if the logging is configured incorrectly. In the future, we should expand this to first check how logging is configured in order to see whether or not the issue should be reported and possibly give a more detailed warning if it is likely the cause of an error. --------- Co-authored-by: Sebastien Poirier <[email protected]> Co-authored-by: seb <[email protected]> * maint: upgrade AMI to Ubuntu 22.04 #512 (#525) * Add `flaml_benchmark` (#528) * dont discard setup_args if it already is a list * Add flaml and flaml_benchmark It is not added to latest since install from latest seems to be broken * Set up alternative way for benchmark mode of flaml This is only temporarily allowed - we expect an easily configurable algorithm, instead of having to carefully install specific dependencies. * limit install, since >2 incompatible * Measure inference time (#532) Add the option to measure inference time (disabled by default) for most frameworks. For those frameworks, inference time is measured capturing both the data loading and the inference. This is done to make things more equal between the different frameworks (as some _need_ to read the file if they don't operator in Python). Inference time is measured multiple times for different batch sizes (configurable). By default, the median is reported in the results file (as it is less sensitive to e.g., cold-starts) but all measured inference times are stored in the predictions folder of a run. For Python frameworks, inference time for in-memory single row predictions is also measured. * Upload to OpenML (#523) Adds a script that allows uploading run results to openml. Additional metadata is stored in the task information to be able to provide a complete description for openml upload. Additional parameters are added to `run_benchmark` to allow runs to automatically be tagged, and to connect to the test server. Also fixes TPOT integration for newer versions, where if a model has no `predict_proba` an `AttributeError` is raised instead of a `RuntimeError`. * Fix a race condition of checking vs adding results (#535) Specifically, adding results was queued in a job executor, while checking results was directly called by the worker threads. If the worker thread checks before the executor had added results, it is possible to get into a deadlock condition. The deadlock arises from the fact that the `stop` condition is never called and the main thread will continue to wait for its END_Q signal. * Add scikit_safe inference time measurement files (#537) * Add scikit_safe inference time measurement files These files have categorical values numerically encoded and missing values imputed, which makes them usable for any scikit-learn algo. * Only generate inference measurement files if enabled * Optionally limit inference time measurements by dataset size (#538) * Add versions 2023 q2 (#539) * Fix versions for June 2023 benchmark * Add 2023Q2 framework tag * Use encoded values for inference * Add us-east-2 AMI * Run docker as root on AWS * Add option to add build options for docker build command * Remove 'infer_speed' artifact as it is not supported in main repo * Fix pandas 2 not compatible with autosklearn 2 see askl#1672 --------- Co-authored-by: github-actions <[email protected]> Co-authored-by: Matthias Feurer <[email protected]> Co-authored-by: seb. <[email protected]> Co-authored-by: Weisu Yin <[email protected]> Co-authored-by: Weisu Yin <[email protected]> Co-authored-by: Eddie Bergman <[email protected]> Co-authored-by: Qingyun Wu <[email protected]> Co-authored-by: Qingyun Wu <[email protected]> Co-authored-by: Robinnibor <[email protected]> Co-authored-by: Erin LeDell <[email protected]> Co-authored-by: Stefan Coors <[email protected]> Co-authored-by: Alan Silva <[email protected]> Co-authored-by: Nick Erickson <[email protected]> Co-authored-by: Leo <[email protected]> Co-authored-by: TrellixVulnTeam <[email protected]> Co-authored-by: seb <[email protected]>
Unfortunately I'm not working on related projects anymore so I'd say do what is better for you guys. If I'll come back one day then I will check and test all that. |
Thanks for letting us know! |
This fixed several issues I created with my previous RP re
docker.py
.Now it runs under $USER, grant permissions and allow
sudo
.