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

Fix re-run smart build bug #165

Merged
merged 6 commits into from
Mar 19, 2022
Merged

Conversation

EricGustin
Copy link
Contributor

@EricGustin EricGustin commented Mar 2, 2022

This PR fixes #162.
There are four cases that this PR accounts for which have been confirmed to work on local machine and Horizon.

  1. User attempts to run smart build when RAI_PATH is set and backends are installed
    • Inform user that backends are already built, tell them where they are built, and notify the user that there is no reason to build.
  2. User attempts to run smart build when RAI_PATH is set, but backends are not installed
    • Inform user that before running smart build, they should unset RAI_PATH
  3. User attempts to run smart build when RAI_PATH is not set and backends are installed
    • Inform user that they need to run smart clean before running smart build
  4. User attempts to run smart build when RAI_PATH is not set and backends are not installed
    • Allow smart build to continue

@EricGustin EricGustin requested review from Spartee and al-rigazzi March 2, 2022 19:57
Copy link
Contributor

@Spartee Spartee left a comment

Choose a reason for hiding this comment

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

Overall this is great. There is one problem which I did not forsee writing up the ticket. The function installed_redisai_backends accounts for the fact that the backends may be installed site-wide at the RAI_PATH which is our plan for smartsim module files (see https://github.com/craylabs/poseidon/issues/176).

This function will stop users from trying to build when they already have the backends built somewhere, but the error message isn't super helpful. They might even try to run smart clean and then find out they are still greeted with the same message if they still have the modulefile loaded.

Leaning on @al-rigazzi and @ashao to make suggestions here. Requesting changes as I think we need to address this first as site-wide installs are part of the plan.

@al-rigazzi
Copy link
Collaborator

Yeah, I think the user should be notified if smart clean will not be effective. One possibility would be to explicitly tell the user where the backends are installed, and what actions could be useful (i.e. build your own SmartSim or change RAI_PATH, maybe?)

@Spartee
Copy link
Contributor

Spartee commented Mar 10, 2022

@al-rigazzi I agree. I think the best plan is to allow them to build and use the backends they want to build seperately if need be. But the default should be to use the set RAI_PATH and REDIS_PATH etc, and prompt the user that the backends are already built at a specific location.

It would be best if there was some way for the user to also install the recommended ML backends provided the RAI version that was installed. this would enabled site-install users to run

# create python env
module load smartsim-0.4.0-rai-1.2.3
smart build --install-ml

this would essentially skip the build process if the backends were module loaded into the environment and then install the correct python dependencies into the user environment.

what do you think @al-rigazzi ?

@al-rigazzi
Copy link
Collaborator

Yes, I agree with @Spartee, that would be cleaner (and automatically documented through smart --help).

@EricGustin EricGustin requested a review from Spartee March 18, 2022 22:45
@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2022

Codecov Report

❗ No coverage uploaded for pull request base (develop@294dc8a). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #165   +/-   ##
==========================================
  Coverage           ?   81.20%           
==========================================
  Files              ?       57           
  Lines              ?     2910           
  Branches           ?        0           
==========================================
  Hits               ?     2363           
  Misses             ?      547           
  Partials           ?        0           

@EricGustin
Copy link
Contributor Author

@Spartee @al-rigazzi changes have been made to address your comments.

Copy link
Contributor

@Spartee Spartee left a comment

Choose a reason for hiding this comment

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

One small change, otherwise good to merge! Feel free to merge after that comment is edited.

if os.environ.get("RAI_PATH"):
if installed_redisai_backends():
logger.error(
f"Backends are already built and loaded at '{CONFIG.redisai}'. There is no need to build."
Copy link
Contributor

Choose a reason for hiding this comment

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

mention that they are specified in the environment at RAI_PATH so the user is aware thats where they are specified.

@EricGustin EricGustin merged commit 3efd2a0 into CrayLabs:develop Mar 19, 2022
@EricGustin EricGustin deleted the inform-backends branch March 19, 2022 21:42
@EricGustin EricGustin mentioned this pull request Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

smart build doesn't inform user which backends are built
4 participants